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

[PHP 7.4 Compat] Remove overloaded offsetExists() #14

Closed
wants to merge 1 commit into from

Conversation

Dave13h
Copy link

@Dave13h Dave13h commented Nov 29, 2019

This blows up on PHP 7.4 because of the change to array_key_exists() which now strictly expects an array.
https://www.php.net/manual/en/migration74.deprecated.php#migration74.deprecated.core.array-key-exists-objects

This method looks like it was only added because of this bug https://bugs.php.net/bug.php?id=40442 which is exclusive to PHP 5.2.1 and was fixed in 5.2.2 - https://www.php.net/ChangeLog-5.php#5.2.2

(copied over from zf1s/zend-registry#1 )

This blows up on PHP 7.4 because of the change to `array_key_exists()` which now strictly expects an `array`.
https://www.php.net/manual/en/migration74.deprecated.php#migration74.deprecated.core.array-key-exists-objects

This method looks like it was only added because of this bug https://bugs.php.net/bug.php?id=40442 which is exclusive to PHP 5.2.1 and was fixed in 5.2.2 - https://www.php.net/ChangeLog-5.php#5.2.2

(copied over from zf1s/zend-registry#1 )
@falkenhawk
Copy link
Member

Whoops, it looks like I have to adjust travis config...

@falkenhawk
Copy link
Member

Since there are a lot more compatibility issues with php7.4 (https://travis-ci.com/zf1s/zf1/jobs/261619983#L350) I am going to convert it into a bigger PR to get rid of them all.

@holtkamp
Copy link
Contributor

holtkamp commented Dec 1, 2019

Since there are a lot more compatibility issues with php7.4

Maybe rector can help with making ZF1 compatible with PHP 7.4, as mentioned in this tweet https://twitter.com/ArkadiuszKondas/status/1200103030823280640 🤓

Please let me know in case help is needed, have a bit of experience writing "own" rectors which might come in handy.

@Megatherium
Copy link

Hi,
I went through all tests yesterday and they all pass now for 7.4 bar one but that seems to be a problem with the sqlite driver and is probably something not as trivial to fix: https://travis-ci.com/Megatherium/zf1/jobs/263793214

I've done a bit more than just the functional stuff (updated docblocks, used more proper assertions, simplified a bit) for every file that I had to touch. Would you like this as a PR? I could also chop it up and do one for each module if that's more to your liking.

Cheers

@falkenhawk
Copy link
Member

@Megatherium great job! Thanks for notifying us. Of course I'd love a PR - since this is a monorepo, one is fine, no need to split it into separate ones for each component.
Did you use a tool for that, e.g. rector as suggested here? Or a different one? I only hope you didn't do all of those changes by hand... I'd only suggest resorting from changing too much stuff especially reformatting docblocks (except where param or return types are fixed - those are welcome changes of course) or other formatting-only changes, in order not to change the original code too much and only when really necessary. But we could continue the discussion in your PR when you set it up.
Thanks again! The open source community is really amazing...

@falkenhawk
Copy link
Member

superseded by #16

@falkenhawk falkenhawk closed this Dec 13, 2019
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.

4 participants