Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Rename file functions by adding a suffix #338

Closed
wants to merge 4 commits into from

Conversation

j3j5
Copy link
Contributor

@j3j5 j3j5 commented Mar 31, 2022

As mentioned on #333 , this is different approach at fixing #253.
I believe this approach is better, cleaner and more scalable since it makes it impossible to confuse the autoloader with the filename and its PSR4 file path.

I've tested it on https://github.com/nepda/phpstan-larastan-bug and it works perfectly.

Let me know your thoughts.

@j3j5
Copy link
Contributor Author

j3j5 commented Mar 31, 2022

I'm not sure what's going on here, running the generator on my branch outputs no changes. Could it be because I'm running on PHP 8.1 and the action is 8.0? I was surprised with the passthru removal but I thought it was coming from master.

@Kharhamel
Copy link
Collaborator

If think you just need to update your documentation folder. You are missing the function 'passthru' whose documentation was recently changed.

@Kharhamel
Copy link
Collaborator

Kharhamel commented Apr 1, 2022

This looks good to me. I will try to get @moufmouf to see this, to make sure this doesn't break some compose magic.

@j3j5 j3j5 force-pushed the array-redeclare-fix-suffix branch from cad1949 to 44169f0 Compare April 1, 2022 18:16
@j3j5
Copy link
Contributor Author

j3j5 commented Apr 1, 2022

You are right, I just did and pushed and updated version. If you are checking out this branch make sure to dump the autoloader again (I'd recommend deleting the composer folder and running composer install again) to make sure the autoloader classes don't have any files pointing to the old filenames.

@codecov-commenter
Copy link

Codecov Report

Merging #338 (44169f0) into master (5bf4aed) will not change coverage.
The diff coverage is 75.00%.

@@            Coverage Diff            @@
##             master     #338   +/-   ##
=========================================
  Coverage     49.43%   49.43%           
  Complexity      313      313           
=========================================
  Files            16       16           
  Lines           791      791           
=========================================
  Hits            391      391           
  Misses          400      400           
Impacted Files Coverage Δ
generator/src/FileCreator.php 0.00% <0.00%> (ø)
generator/src/ComposerJsonEditor.php 44.82% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5bf4aed...44169f0. Read the comment docs.

@moufmouf
Copy link
Member

moufmouf commented Apr 1, 2022

Woo, I catched up a looooong series of issues. Thanks a lot @j3j5 for the PR and congrats for understanding the root cause of the issue!

The issue
I do understand how this will solve the problem, so 👍 for the fix.

There is just one thing that frightens me a bit. Anyone relying on Safe but not using the autoloader (but instead including files manually) will have breaking changes. I absolutely don't know if some people are doing this in the wild or not.

I mean, I would never do that myself, but you can see some crazy code out there (maybe someone trying to overoptimize things could try to manually import files?) Or am I overthinking this?

#333 looks kind of ok to me actually. What are the particular issues with #333 ?

@j3j5
Copy link
Contributor Author

j3j5 commented Apr 1, 2022

It is a risk indeed, if you decide to go with this solution you may want to make a major release just to make sure, although I doubt many people do manually import the files, it is a possibility and we'll be breaking their workflow with this solution (mandatory xkcd).

As for #333, latest changes on master (specifying iterable types on array returns) did already break the branch. It certainly is less disruptive than this and I'm sure we can figure out a more resilient way (regex?) to make sure all array are converted to \array, but you still may want to add some sort of test to check it over time. With this solution you remove this necessity and make sure it won't happen again.

On the other hand, I'm writing this and thinking that the solution may actually be easier and less disruptive. What if we just rename the file for array into arrays.php (with an s, just like strings.php). I know the module is called array on the docs, but it's probably easier (and less likely to change in the future) to add the str_replace here than to replace all return types that include array in all the forms that it may take in the future. It may break things on 3rd party code but the fix will be easier for them as well.

As I said on the other PR, at the end of the day y'all are the maintainers of the package so the solution is up to you. I'm happy just getting rid of the error on my packages 😛

@Kharhamel Kharhamel mentioned this pull request Apr 5, 2022
@Kharhamel
Copy link
Collaborator

Closing this since #333 was merged instead

@Kharhamel Kharhamel closed this Apr 19, 2022
@Kharhamel Kharhamel reopened this Apr 19, 2022
@Kharhamel
Copy link
Collaborator

@j3j5 Do you think you can solve the conflicts? Once it is done, i will think some people to test this on a real project and then I will create an alpha

@j3j5
Copy link
Contributor Author

j3j5 commented Apr 28, 2022

Closing in favor of #350

@j3j5 j3j5 closed this Apr 28, 2022
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants