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

feature(php): Require PHP 5.4+ #7475

Merged
merged 1 commit into from Nov 11, 2014
Merged

feature(php): Require PHP 5.4+ #7475

merged 1 commit into from Nov 11, 2014

Conversation

ewinslow
Copy link
Contributor

PHP 5.3 is currently end-of-life and 5.4 will have only ~1 year of security patches
when Elgg 1.10 is scheduled to be released.

Fixes #7090

PHP 5.3 is currently end-of-life and 5.4 will have only ~1 year of security patches
when Elgg 1.10 is scheduled to be released.

Fixes Elgg#7090
@ewinslow
Copy link
Contributor Author

Note that one feature in particular I'd really like to have for #7474 is lexical $this support for closures, which is not available in 5.3.

@ewinslow
Copy link
Contributor Author

@brettp, @mrclay, @hypeJunction -- WDYT?

@hypeJunction
Copy link
Contributor

Fine by me. I say we give a fair warning to the community, make a blog post or something, so people can work with their hosting providers to upgrade the PHP.

@ewinslow
Copy link
Contributor Author

Sounds good. I'm happy to do it once this goes in.

I believe this is a no-brainer, but since it's such a fundamental requirement change and we never really got closure when this was last discussed, I think I'd like to see 3 LGTMs (4 including me) and no blockers before pulling it in.

@mrclay
Copy link
Member

mrclay commented Nov 11, 2014

Lgtm

@hypeJunction
Copy link
Contributor

LGTM. Should we perhaps short circuit the installer in version is lower?

@ewinslow
Copy link
Contributor Author

I already updated the min version check in the installer which should issue a visible warning. Also composer so complain about any attempt to install on php < 5.4. What else did you have in mind?

@ewinslow
Copy link
Contributor Author

@Elgg/owners @Elgg/contributors ... Just to make sure everyone is aware in case notifications aren't turned on or something.

@Srokap
Copy link
Contributor

Srokap commented Nov 11, 2014

LGTM

1 similar comment
@juho-jaakkola
Copy link
Member

LGTM

@ewinslow
Copy link
Contributor Author

OK! We've got the LGTMs. I will write the blog post and merge this once that is done unless anyone objects...

@jeabakker
Copy link
Member

why are some requirements 5.6?

@Srokap
Copy link
Contributor

Srokap commented Nov 11, 2014

Those are only tests and sample manifest. I think there's no harm in promoting even newer PHP version in docs.

@ewinslow
Copy link
Contributor Author

They are not requirements. Those are just example plugin manifests. Honestly, I made them 5.6 so we wouldn't have to change them for a while :). But I think it also serves to show that you can require a higher version of php for your plugins than core

@jeabakker
Copy link
Member

ok then lgtm ;)

ewinslow added a commit that referenced this pull request Nov 11, 2014
feature(php): Require PHP 5.4+
@ewinslow ewinslow merged commit 8a8079b into Elgg:1.x Nov 11, 2014
@ewinslow ewinslow deleted the php-54 branch November 11, 2014 15:54
@ewinslow
Copy link
Contributor Author

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants