-
Notifications
You must be signed in to change notification settings - Fork 8
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
patches php 81 #35
base: master
Are you sure you want to change the base?
patches php 81 #35
Conversation
Hi @mashb1t, maybe I missed something (just a quick review), but what exactly 'made the code compatible with PHP8.1'? beside that: But what IMHO is not acceptable is the last commit 14c3e6f. This way the module can't be included anymore under another name than the one from Module::NAME, or several times with different config. PS: it would be nice to separate formatting from functional changes in commits. Makes the process much clearer and easier for reviewers. Cc: @eluhr |
Hi @handcode just to be clear: the changes do not claim to establish full 8.1 compatibility of the project - we just fixed some issues we had in our implementation when lifting it up to 8.1 - there still might be some incompatibilities left in other parts of the code we do not use. In our case it was mostly adding the null/empty checks as of 8.1 functions like ltrim & str_replace do not support null as a parameter any longer and in some cases the parent functions were called with null parameters causing exceptions when running under php 8.1. You're right about your concerns about 14c3e6f - i did another implementation with several fallbacks in cad1f1d where it uses the name only as a last resort when getting the module from the loadedModules map or the controller failes. I hope that's acceptable |
Hey @schmunk42 / @handcode,
we made the code compatible with PHP8.1. It would be great if you could review and merge the changes if you're fine with the adjustments.