Skip to content

Fix dynamic properties warnings #2860

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
Merged
Levdbas merged 3 commits into from
Jan 26, 2024
Merged

Fix dynamic properties warnings #2860

Levdbas merged 3 commits into from
Jan 26, 2024

Conversation

@trsteel88
Copy link

@trsteel88 trsteel88 commented Dec 4, 2023

Related:

Issue

This removes the warnings that flood a site on newer versions of PHP

Solution

Implement the #[\AllowDynamicProperties] attribute. This is not a BC break as older versions of PHP would see this as a comment and ignore it.

Impact

None.

@Levdbas Levdbas added the 1.x label Dec 6, 2023
@Levdbas Levdbas added this to the 1.24.0 milestone Dec 14, 2023
gchtr
gchtr requested changes Jan 6, 2024
View reviewed changes
Copy link
Member

@gchtr gchtr left a comment

Choose a reason for hiding this comment

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

Thanks for the pull request!

In Timber 2.0, we also have #[\AllowDynamicProperties] on Timber\PostType.

timber/src/PostType.php

Line 11 in 7992eee

#[AllowDynamicProperties]

Because Timber\PostType doesn’t extend Timber\Core, I think we should add it there as well.

@trsteel88
Copy link
Author

trsteel88 commented Jan 6, 2024

Good catch @gchtr. That's been updated in this PR

@trsteel88 trsteel88 requested a review from gchtr January 6, 2024 10:28
@trsteel88
Copy link
Author

trsteel88 commented Jan 15, 2024

Any chance of getting this merged and tagged @gchtr?

@gchtr gchtr mentioned this pull request Jan 25, 2024
Merged
@gchtr
Copy link
Member

gchtr commented Jan 25, 2024 •
edited
Loading

Any chance of getting this merged and tagged @gchtr?

Yes, working on it. Just had to fix some tests and add testing for 8.2 to confirm that all Dynamic Property Warnings in Timber are actually gone. See #2890.

nlemoine
nlemoine approved these changes Jan 26, 2024
View reviewed changes
Copy link
Member

@nlemoine nlemoine 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 👍

@Levdbas Levdbas merged commit 1dc2589 into timber:1.x Jan 26, 2024
@gchtr
Copy link
Member

gchtr commented Jan 31, 2024

This is now released in https://github.com/timber/timber/releases/tag/1.24.0

@trsteel88 trsteel88 deleted the fix-dynamic-properties-warning branch February 5, 2024 05:29
@trsteel88
Copy link
Author

trsteel88 commented Feb 5, 2024

Thanks @gchtr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gchtr gchtr gchtr approved these changes

@nlemoine nlemoine nlemoine approved these changes

Assignees

No one assigned

Labels

1.x

Projects

None yet

Milestone

1.24.0

Development

Successfully merging this pull request may close these issues.

4 participants

@trsteel88 @gchtr @nlemoine @Levdbas