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

Fix i18nTextCollector produces corrupt output / namespaces when running under PHP8.0 #10228

Merged
merged 2 commits into from
May 30, 2022
Merged

Fix i18nTextCollector produces corrupt output / namespaces when running under PHP8.0 #10228

merged 2 commits into from
May 30, 2022

Conversation

bimthebam
Copy link
Contributor

This PR fixes the buggy behaviour of the i18nTextCollector (#10227 ) when running under PHP8.0

@michalkleiner
Copy link
Contributor

Thanks for the contribution @bimthebam.
Since the tests are passing now and were passing before, do you think you can have a look into the tests and update any of the tests (or provide a new one) specifically targetting the PHP8 behaviour that was broken before your PR?

@bimthebam
Copy link
Contributor Author

Hi @michalkleiner I've just updated i18nTextCollectorTest::testCollectFromCodeNamespace() which will now fail when running with the unpatched version of i18nTextCollector under PHP8.0

@bimthebam bimthebam changed the title Fix #10227 Fix i18nTextCollector produces corrupt output / namespaces when running under PHP8.0 Feb 9, 2022
michalkleiner
michalkleiner approved these changes Feb 9, 2022
View reviewed changes
Copy link
Contributor

@michalkleiner michalkleiner left a comment

Choose a reason for hiding this comment

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

Even though I didn't run this locally I'm ok with this change since it doesn't break the existing tests and the change to the test (referencing the Versioned class by its FQDN) should not change the logic of it

GuySartorelli
GuySartorelli approved these changes May 30, 2022
View reviewed changes
Copy link
Member

@GuySartorelli GuySartorelli 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. Have tested locally and not encountered any issues.

@GuySartorelli GuySartorelli merged commit d37dded into silverstripe:4 May 30, 2022
GuySartorelli pushed a commit to GuySartorelli/silverstripe-framework that referenced this pull request Jul 6, 2022
@bimthebam @GuySartorelli
Fix i18nTextCollector produces corrupt output / namespaces when runni…
d2d6dd1 
…ng under PHP8.0 (silverstripe#10228)

* FIX i18nTextCollector produces corrupt output / namespaces when running under PHP8.0
@blueskies79 blueskies79 mentioned this pull request Sep 27, 2022
Closed
GuySartorelli pushed a commit to creative-commoners/silverstripe-framework that referenced this pull request Sep 29, 2022
@bimthebam @GuySartorelli
Fix i18nTextCollector produces corrupt output / namespaces when runni…
e24fb3f 
…ng under PHP8.0 (silverstripe#10228)

* FIX i18nTextCollector produces corrupt output / namespaces when running under PHP8.0
This was referenced Sep 29, 2022
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuySartorelli GuySartorelli GuySartorelli approved these changes

@michalkleiner michalkleiner michalkleiner approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@bimthebam @michalkleiner @GuySartorelli