Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

mashb1t
Copy link

@mashb1t mashb1t commented Dec 9, 2022

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.

@handcode
Copy link
Member

handcode commented Dec 9, 2022

Hi @mashb1t,

maybe I missed something (just a quick review), but what exactly 'made the code compatible with PHP8.1'?

beside that:
the empty() checks looks ok, we have to look closer at the exception handling, but it should also fit.

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

@boehmi1988
Copy link

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

@schmunk42 schmunk42 requested a review from eluhr January 17, 2023 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants