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

FIX PHP 5.4 compat for using function return value in write context #411

Closed

Conversation

robbieaverill
Copy link
Contributor

@robbieaverill robbieaverill commented Mar 18, 2019

This restores compatibility with older PHP versions. v1.15.0 still allows PHP 5.3 or greater.

It'd be great if you could release this in a 1.15.x patch, I realise it'll be slightly annoying since other commits exist in master already which bump these requirements.

You can see the error here: https://travis-ci.org/lekoala/silverstripe-debugbar/jobs/508134678#L528

@barryvdh
Copy link
Collaborator

So I would need a separate branch for 1.15 and then merge this?

@robbieaverill
Copy link
Contributor Author

robbieaverill commented Mar 19, 2019

Yeah that’d be great if you wouldn’t mind. I’ll rebase the PR afterwards

@leomoty
Copy link
Contributor

leomoty commented Mar 19, 2019

From my experience this is not entirely true for php 5.3.3 (or lower), which is the case I still have to handle in a legacy system.

the array [ ] syntax is only supported in php 5.4 and it is used in the same file you changed:
leomoty@bb7d7e5

$fileinfo->getExtension() is only available after php 5.3.6:

leomoty@51df756

Anyways, my point is, either bump composer.json support for >5.4.0 or you are likely to need my extra fixes.

@robbieaverill
Copy link
Contributor Author

It seems strange to remove support for PHP 5.4 in a patch release (to me). Perhaps either someone using PHP 5.3 needs to whip over and fix the incompatible syntax, or revert any commits that broke PHP 5.3 compatibility. In my case I'm also (very slightly) concerned with PHP 5.3 as well as PHP 5.4.

@leomoty
Copy link
Contributor

leomoty commented Mar 19, 2019

If you check the commits I linked, I fixed the array square brackets, getExtension and the ini_get. I didn't expect this to be accepted upstream, so that is why I went that route.

And I agree that it seems pretty weird, I just meant that the repository in that specific tag wasn't really PHP 5.3.0 compliant.

@robbieaverill
Copy link
Contributor Author

robbieaverill commented Mar 19, 2019

@leomoty maybe you could create a pull request from your commits in order to restore compatibility for it =) I'd be happy to review it

@leomoty
Copy link
Contributor

leomoty commented Mar 19, 2019

@robbieaverill or you can just add my commits to your PR? either way, we need to wait for the 1.15 branch, right?

@robbieaverill
Copy link
Contributor Author

Thanks @leomoty, I've cherry picked your commits into this pull request

@robbieaverill
Copy link
Contributor Author

@barryvdh anything I can do to get this merged in?

@robbieaverill
Copy link
Contributor Author

Hi @barryvdh, not trying to be annoying here, but would you be able to merge this? If you'd like extra maintenance help, I'm happy to get involved as well.

@robbieaverill
Copy link
Contributor Author

@barryvdh feel free to reopen this PR if you'd like to look at it in the future, for now we'll drop support for PHP 5.3 and 5.4 instead. Thanks.

@leomoty
Copy link
Contributor

leomoty commented Jun 6, 2019

I don't have that option, but i'll just keep using my fork till we are finally able to get rid of PHP 5.3

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.

None yet

3 participants