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.

Sign up for GitHub

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

Jump to bottom

TASK: PHP 7.4 compatibility #2804

Merged
merged 7 commits into from Jan 15, 2020
Merged

TASK: PHP 7.4 compatibility #2804

merged 7 commits into from Jan 15, 2020

Conversation

kdambekalns
Copy link
Member

@kdambekalns kdambekalns self-assigned this Dec 2, 2019
@kdambekalns
Copy link
Member Author

Haha, this still needs PHP 7.4 to be enabled for Travis… feel free, I have to run now.

bwaidelich
bwaidelich approved these changes Dec 2, 2019
View reviewed changes
Copy link
Member

@bwaidelich bwaidelich left a comment

Choose a reason for hiding this comment

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

👍 by reading

@markusguenther markusguenther mentioned this pull request Dec 2, 2019
Closed
3 tasks
@kdambekalns
TASK: Enable PHP 7.4 for Travis CI
ec04272 
This change

- removes PostgreSQL tests with PHP 7.1
- adds tests on PHP 7.3
- adds tests on PHP 7.4
- switches PostgreSQL 9.4 to 9.5 to tests
@kdambekalns
TASK: Switch to Ubuntu Bionic and stable PHP 7.4
4fbc2c5
@kdambekalns
TASK: For tests switch PostgreSQL 9.4 to 9.5
8b9fec8 
PostgreSQL 9.4 will stop being supported in February 2020.
@kdambekalns
Copy link
Member Author

kdambekalns commented Dec 3, 2019 •
edited

FYI: The test failures will be fixed with Behat/Transliterator#29

albe
albe approved these changes Dec 3, 2019
View reviewed changes
Copy link
Member

@albe albe left a comment

Choose a reason for hiding this comment

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

Looks good, but not sure about those test failures - something Gd, rather than Behat/Transliterator, no?

@kdambekalns
Copy link
Member Author

Haha, right. I only knew Array and string offset access syntax with curly braces is deprecated but the GD error is probably a result of switching to Bionic. Seems GD needs to be installed during setup…

@kdambekalns
Copy link
Member Author

Ok, still waiting for behat/transliterator PR #29 to be released…

Maybe go this way, too? https://github.com/KnpLabs/DoctrineBehaviors/pull/468/files#diff-c5f7d7b9e0f14fa580860143f56f3b7f

@kdambekalns
Copy link
Member Author

Tried symfony/string really quick this morning…

There were 7 failures:

1) [NeosCR]\Unit\UtilityTest::renderValidNodeNameWorks with data set #2 ('汉语', 'yi-yu')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'yi-yu'
+'han-yu'

/…/Packages/Neos/Neos.ContentRepository/Tests/Unit/UtilityTest.php:54

2) [NeosCR]\Unit\UtilityTest::renderValidNodeNameWorks with data set #5 ('ភាសាខ្មែរ', 'bhaasaakhmaer')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'bhaasaakhmaer'
+'node-270ae7607f9a555aac6ff07f02652788'

/…/Packages/Neos/Neos.ContentRepository/Tests/Unit/UtilityTest.php:54

3) [NeosCR]\Unit\UtilityTest::renderValidNodeNameWorks with data set #6 ('ภาษาไทย', 'phaasaaaithy')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'phaasaaaithy'
+'phas-a-thiy'

/…/Packages/Neos/Neos.ContentRepository/Tests/Unit/UtilityTest.php:54

4) [NeosCR]\Unit\UtilityTest::renderValidNodeNameWorks with data set #7 ('العَرَبِية', 'l-arabiy')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'l-arabiy'
+'al-arabit'

/…/Packages/Neos/Neos.ContentRepository/Tests/Unit/UtilityTest.php:54

5) [NeosCR]\Unit\UtilityTest::renderValidNodeNameWorks with data set #9 ('한국어', 'hangugeo')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'hangugeo'
+'hangug-eo'

/…/Packages/Neos/Neos.ContentRepository/Tests/Unit/UtilityTest.php:54

6) [NeosCR]\Unit\UtilityTest::renderValidNodeNameWorks with data set #11 ('မြန်မာဘာသာ', 'm-n-maabhaasaa')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'m-n-maabhaasaa'
+'node-e035ba647b2805199cfab4478fff8cf1'

/…/Packages/Neos/Neos.ContentRepository/Tests/Unit/UtilityTest.php:54

7) [NeosCR]\Unit\UtilityTest::renderValidNodeNameWorks with data set #12 (' हिन्दी', 'hindii')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'hindii'
+'hindi'

/…/Packages/Neos/Neos.ContentRepository/Tests/Unit/UtilityTest.php:54

I can imagine the previous results were wrong, so… how do we check that?

@kdambekalns
Copy link
Member Author

See symfony/symfony#35061

@kdambekalns kdambekalns mentioned this pull request Dec 20, 2019
Merged
@bwaidelich
Copy link
Member

@kdambekalns at least this one is weird

--- Expected
+++ Actual
@@ @@
-'m-n-maabhaasaa'
+'node-e035ba647b2805199cfab4478fff8cf1'

In general I don't think we should change the transliteration behavior in minor (or even patch) level releases as it can have a great effect.

For the long run I would prefer to avoid any 3rd party library and use the built-in \Transliterator instead with a sane configuration, possibly locale-based (see discussion on neos/flow-development-collection#1861 (comment))

@kdambekalns
Copy link
Member Author

use the built-in \Transliterator instead with a sane configuration

Hah, been there, done that. If you can find that "sane configuration", ideally backed by some documentation that humans can understand, go ahead.

In the meantime, let's see what pinning behat/transliterator gives us (after all, the package changes verry, very, very, very, very slowly) - and if merged with the pinned version, open an issue to remove said pin as soon as a new release of behat/transliterator has been done.

@kdambekalns
Copy link
Member Author

let's see what pinning behat/transliterator gives us

I should have seen that coming. Of course using a dev version is impossible unless whitelisted one way or the other in your root manifest. Which we could do for our base and development distributions, but expecting everyone out there to adjust their's seems a bit too much.

@jonnitto
Copy link
Member

@kdambekalns Looks like we are ready for 7.4 https://github.com/Behat/Transliterator/releases/tag/v1.3.0

@kdambekalns kdambekalns merged commit 10f17ad into neos:4.3 Jan 15, 2020
@kdambekalns
Copy link
Member Author

🎉

@kdambekalns kdambekalns deleted the task/php-74-compatibility branch January 15, 2020 08:57
mhsdesign added a commit to mhsdesign/neos-development-collection that referenced this pull request Oct 26, 2022
@mhsdesign
BUGFIX: 3585 Fusion Runtime @apply with integer indexed array
26b5204 
starting with PHP 7.4 it's not cool to do offset access on integers (which returns null always)

I did a little digging, and we have this skip keys logic 3 times in the runtime (probably not extracted for performance reasons)

2 times this logic needs to be adjusted to only operate on strings. Those two snippets where introduced with neos#2738

And 1 time this logic already has an is_string check. https://github.com/neos/neos-development-collection/blob/046e5d02750af0ebf33cc52663799ce09683ba37/Neos.Fusion/Classes/Core/Runtime.php#L667
This was adjusted when 7.4 compatibility was introduced via neos#2804 but the other two references where not adjusted
@mhsdesign mhsdesign mentioned this pull request Oct 26, 2022
Merged
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bwaidelich bwaidelich bwaidelich approved these changes

@kitsunet kitsunet kitsunet approved these changes

@albe albe albe approved these changes

@robertlemke robertlemke Awaiting requested review from robertlemke

Assignees

@kdambekalns kdambekalns

Labels
T: PHP
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@kdambekalns @bwaidelich @jonnitto @kitsunet @albe