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

Remedy deprecated dynamic variable declaration #354

Merged
merged 10 commits into from
Jan 15, 2024

Conversation

alex-dna
Copy link

This should prevent warning to be triggered by php 8.2

@alex-dna alex-dna mentioned this pull request Dec 20, 2023
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.

dynamic properties are public - these new properties must also be public to avoid backwards compatibility issues.

Please remove the default values - there are no default for dynamic properties, which these are replacing.

Please also target the 4.0 branch - ideally this change will be released as a patch release.

@michalkleiner
Copy link
Contributor

michalkleiner commented Dec 22, 2023

Thought I might be able to batch commit the suggestions from GH UI, but no dice.

@GuySartorelli
Copy link
Member

@alex-dna There are some requested changes on this PR - do you still want to have this change merged in?
If so, please make those changes. If the request changes aren't made in a few weeks I'll close this PR.

alex-dna and others added 2 commits January 15, 2024 15:05
Co-authored-by: Michal Kleiner <mk@011.nz>
Co-authored-by: Michal Kleiner <mk@011.nz>
@alex-dna alex-dna changed the base branch from 4 to 4.0 January 15, 2024 02:07
@alex-dna
Copy link
Author

@GuySartorelli I've committed the requested changes. Thanks @michalkleiner . This is ow targetting the right branch I think.

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 okay to me now. Thanks for that.

@michalkleiner do you have any concerns? (other than the merge commits in there, but I'll just squash-and-merge if you're happy with it)

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.

All good from my perspective, thanks @alex-dna and @GuySartorelli

@GuySartorelli GuySartorelli merged commit 49a829f into silverstripe:4.0 Jan 15, 2024
10 checks passed
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