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

Fixes for PHP 7.3 #2887

Merged
merged 5 commits into from
Mar 13, 2019
Merged

Fixes for PHP 7.3 #2887

merged 5 commits into from
Mar 13, 2019

Conversation

michael-e
Copy link
Member

Re: #2884

PHP 7.3 uses PCRE2, which requires hyphens to be escaped unless they either symbolize a character range or occur at the beginning or end of a character class. Otherwise PHP will throw "Compilation failed: invalid range in character class at offset ...".

Escaping is backwards-compatible.
PHP 7.3 introduces a "JSONException" class, so we must rename ours in order to prevent the following error: "Cannot declare class JSONException, because the name is already in use ...".

This commit also updates the Composer `ClassLoader.php`.
@nitriques nitriques added this to the 2.7.8 milestone Mar 7, 2019
symphony/lib/toolkit/class.general.php Show resolved Hide resolved
vendor/composer/ClassLoader.php Outdated Show resolved Hide resolved
nitriques added a commit that referenced this pull request Mar 7, 2019
This commits makes sure we do not load our custom JSONException class if
a native implementation already exists.

Re #2887
This commits makes sure we do not load our custom JSONException class if
a native implementation already exists.
@michael-e
Copy link
Member Author

I think that we don't need to run composer dump-autoload anymore, because autoload_classmap.php wouldn't change. (FYI, if I try, I get the same programmatic differences in ClassLoader.php as before.)

@michael-e michael-e requested a review from nitriques March 7, 2019 23:39
@michael-e
Copy link
Member Author

@nitriques: I added a commit with the requirements change. This might cause some headache for extension developers. Unfortunately, we never officially added the PHP compatibility attributes to the extension meta XML schema.

However, my big system (on PHP 7.3) uses a lot of extensions, and I have only found a few issues (and sent PRs). And the more important point is: Symphony LTS should officially run on the current PHP version. I'd rather risk a bug than make Symphony look "discontinued".

Copy link
Member

@nitriques nitriques left a comment

Choose a reason for hiding this comment

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

LGTM!

@nitriques
Copy link
Member

Unfortunately, we never officially added the PHP compatibility attributes to the extension meta XML schema.

We did it ;) it is not released yet...

Symphony LTS should officially run on the current PHP version.

Yes, but we can't drop anything!

@nitriques nitriques merged commit 2eee808 into symphonycms:2.7.x Mar 13, 2019
@michael-e michael-e deleted the php-7.3 branch March 14, 2019 00:00
@froschdesign
Copy link

@nitriques

Yes, but we can't drop anything!

Sure you can! Or does someone pay you to support the dead PHP versions?

And it is a wrong signal to the users of the CMS, because if you support the old PHP versions the user believes he does not have to do anything. But it is a security risk!

As a (open source) project without charge, it should not be responsible for problems with unsupported PHP versions.

@froschdesign
Copy link

nitriques added a commit to DeuxHuitHuit/symphonycms that referenced this pull request Mar 29, 2019
This commits makes sure we do not load our custom JSONException class if
a native implementation already exists.

Re symphonycms#2887

Picked from 590413b
nitriques added a commit that referenced this pull request Apr 8, 2019
This commits makes sure we do not load our custom JSONException class if
a native implementation already exists.

Re #2887

Picked from 590413b
Picked from 265f5f9
nitriques added a commit to DeuxHuitHuit/symphonycms that referenced this pull request Apr 8, 2019
This commits makes sure we do not load our custom JSONException class if
a native implementation already exists.

Re symphonycms#2887

Picked from 590413b
Picked from 265f5f9
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants