Skip to content

[PropertyAccess] Non-standard adder/remover methods #13137

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

Merged

Conversation

Korbeil
Copy link
Contributor

@Korbeil Korbeil commented Feb 14, 2020

Will add documentation about non-standard adder/remover methods for PropertyAccessor.

Related to #13023 and symfony/symfony#9336
This is not a complete documentation for related issues, but this will introduce one of the possible new use.

@Korbeil Korbeil changed the title Non-standard adder/remover methods [PropertyAccess] Non-standard adder/remover methods Feb 14, 2020
@HeahDude HeahDude added this to the 4.4 milestone Feb 19, 2020
@wouterj wouterj added the Waiting Code Merge Docs for features pending to be merged label Oct 22, 2020
@wouterj
Copy link
Member

wouterj commented Oct 22, 2020

Fyi: Marked as "waiting code merge" as I think it depends on symfony/symfony#38515

@Korbeil
Copy link
Contributor Author

Korbeil commented Oct 22, 2020

@wouterj I does not depends on that PR, it was dependant on symfony/symfony#30704 which I finished a long time ago ^^
The PR you linked is only DX to make that overwrite easier (but not covered in this documentation).

@wouterj
Copy link
Member

wouterj commented Oct 22, 2020

Oh, I'm sorry. I thought that PR only did changes to PropertyInfo and missed the big changes to the PropertyAccessor.

@wouterj wouterj removed the Waiting Code Merge Docs for features pending to be merged label Oct 22, 2020
Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thank you @Korbeil! I've proposed some changes to slightly improve the code example.

Can you please also add a versionadded directive like:

.. versionadded:: 5.1

    Support for non-standard adder/remover methods was introduced in Symfony 5.1.

And at last, please rebase this PR on the 5.1 branch (using git rebase --onto origin/5.1 origin/master).

If you don't have time to work on these changes (or something is unclear), please tell me and I'll finish this PR.

@Korbeil Korbeil changed the base branch from master to 5.1 October 23, 2020 11:42
@Korbeil Korbeil force-pushed the feature/add-non-standard-adder-remover-methods branch from 62f43d9 to b710816 Compare October 23, 2020 11:42
@Korbeil Korbeil force-pushed the feature/add-non-standard-adder-remover-methods branch from b710816 to bf6cca9 Compare October 23, 2020 11:45
@Korbeil
Copy link
Contributor Author

Korbeil commented Oct 23, 2020

@wouterj I made all requested changes

Copy link
Member

@wouterj wouterj left a comment

Choose a reason for hiding this comment

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

Thank you for the quick response @Korbeil!

@wouterj
Copy link
Member

wouterj commented Oct 31, 2020

Hi @Korbeil. I was about to merge this PR but I'm lost in all the core commits and PRs again.. This PR has got the "4.4" milestone, which makes me think it should be merged in 4.4. But it appears like the feature is introduced in 5.1. Can you please confirm which version should be used?

@Korbeil
Copy link
Contributor Author

Korbeil commented Nov 1, 2020

@wouterj, As you can see in related PR: symfony/symfony#30704 It was firstly tagged in v5.1.0-BETA1

Or maybe it's my github enhanced addon that shows it ?
image

@wouterj
Copy link
Member

wouterj commented Nov 1, 2020

Oh, I don't have that feature. That seems extremely useful 😄

image

You probably submitted this PR when 4.4 was in development, causing a doc team member to flag it as 4.4 a bit too quickly. I'll merge in 5.1 then, thanks for confirming.

@wouterj wouterj modified the milestones: 4.4, 5.1 Nov 1, 2020
@wouterj wouterj merged commit 7370af6 into symfony:5.1 Nov 5, 2020
@wouterj
Copy link
Member

wouterj commented Nov 5, 2020

Yay, and finally it is merged. Thanks for your patience & contributions @Korbeil!

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

Successfully merging this pull request may close these issues.

5 participants