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

Php7 migration #9

Open
wants to merge 27 commits into
base: php7-migration
Choose a base branch
from

Conversation

macintoshplus
Copy link

@macintoshplus macintoshplus commented Apr 5, 2019

This PR replace all others (#8, #7 , #6 , #5 , #4) and more.

Please after "squash and merge", add the tag v4.0.0 for this version

Copy link
Owner

@znk3r znk3r left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi Jean-Baptiste, thanks for the collaboration.
The changes are really similar to what I have been making on my own, but it includes more breaking changes than I'm comfortable with.
Let me explain. My plan is to create a 4.0.0 version fully compatible with the previous one. Sadly, version 3 was designed for PHP 3, which means all variables and methods have public visibility. As you probably know, legacy applications have the bad habit of ignoring visibility from comments and accessing the variables anyway. My other concern is the change in the global variables, because other code may relay on them being arrays instead of strings.
I would like to keep those details in version 4.0.0 as a drop-in replacement for the previous version, while fix the visibility, error handling, uses of static and add proper testing on a future 5.x version.

@macintoshplus
Copy link
Author

macintoshplus commented Apr 8, 2019

Thanks for your review and share your strategy. The visibility is based on the comment information but much visibility have been changed after (variables and methods).

  1. You can make all your change in the php7-migration branch or another with the backward compatibility.

  2. I can recreate this PR for the v5.x branch (after you create-it).

It's an acceptable plan or you want only one version?

The global variables have been updated because the files paths are outdated with the composer autoloader.

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.

2 participants