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

[Serializer] Fix access to private properties/getters when using the @Ignore annotation #52680

Merged
merged 1 commit into from Nov 24, 2023
Merged

[Serializer] Fix access to private properties/getters when using the @Ignore annotation #52680

merged 1 commit into from Nov 24, 2023

Conversation

mtarld
Copy link
Contributor

@mtarld mtarld commented Nov 22, 2023

Q A
Branch? 5.4
Bug fix? yes
New feature? no
Deprecations? no
Issues Fix #52673 #49710
License MIT

@mtarld mtarld requested a review from dunglas as a code owner November 22, 2023 03:54
@carsonbot carsonbot added this to the 5.4 milestone Nov 22, 2023
@OskarStark OskarStark changed the title [Serializer] Fix access to private properties/getters when using the Ignore attribute [Serializer] Fix access to private properties/getters when using the @Ignore annotation Nov 22, 2023
@nesl247
Copy link

nesl247 commented Nov 22, 2023

@mtarld am I misunderstanding the documentation though? It says

All attributes are included by default when serializing objects. There are two options to ignore some of those attributes.

Which to mean means even private and protected. While I think the fix here makes sense, it doesn't align with the documentation. Should the documentation also be updated to say only publicly accessible attributes are included? Or is that what attribute means and I am missing the definition?

@stof
Copy link
Member

stof commented Nov 22, 2023 •
edited

Private or protected properties are included if they have getters to access them publicly.

@nesl247
Copy link

nesl247 commented Nov 22, 2023

Private or protected properties are included if they have getters to access them publicly.

That's what I thought. Then the documentation should be updated to specify that instead of all attributes correct?

@mtarld
Copy link
Contributor Author

mtarld commented Nov 23, 2023

Indeed, we could go with something like: "All accessible attributes are included by default when serializing objects.".

WDYT? Up for a PR?

@nesl247
Copy link

nesl247 commented Nov 23, 2023

Indeed, we could go with something like: "All accessible attributes are included by default when serializing objects.".

WDYT? Up for a PR?

Yes. I’m out of the office until Tuesday, but when I get back I’m happy to submit a PR.

@nicolas-grekas
Copy link
Member

PHP Parse error: syntax error, unexpected 'string' (T_STRING), expecting function (T_FUNCTION) or const (T_CONST) in /home/runner/work/symfony/symfony/src/Symfony/Component/Serializer/Tests/Normalizer/AbstractObjectNormalizerTest.php on line 498

@mtarld
Copy link
Contributor Author

mtarld commented Nov 24, 2023

Failures should be fixed in #52713

@nicolas-grekas
Copy link
Member

Thank you @mtarld.

@nicolas-grekas nicolas-grekas merged commit 1da3a5c into symfony:5.4 Nov 24, 2023
1 of 11 checks passed
@mtarld mtarld deleted the fix/ignore branch November 24, 2023 11:44
This was referenced Nov 26, 2023
Merged
Merged
@mtarld mtarld mentioned this pull request Nov 27, 2023
Closed
@nesl247 nesl247 mentioned this pull request Nov 28, 2023
Merged
OskarStark added a commit to symfony/symfony-docs that referenced this pull request Nov 28, 2023
@OskarStark
minor #19194 fix: serializer includes only accessible attributes by d…
d3710d1 
…efault (nesl247)

This PR was submitted for the 6.4 branch but it was merged into the 5.4 branch instead.

Discussion
----------

fix: serializer includes only accessible attributes by default

Updates documentation based upon the discussion here: symfony/symfony#52680

Commits
-------

674ff7d fix: serializer includes only accessible attributes by default
@mtarld mtarld mentioned this pull request Nov 29, 2023
Merged
nicolas-grekas added a commit that referenced this pull request Nov 29, 2023
@nicolas-grekas
bug #52786 [Serializer] Revert allowed attributes fix (mtarld)
963cccd 
This PR was merged into the 5.4 branch.

Discussion
----------

[Serializer] Revert allowed attributes fix

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        |
| License       | MIT

Revert #52767 and #52680, as it does not cover some specific use cases, and can't be fixed in time.

I'll attempt to fix it in a better way, taking more time.

Commits
-------

1dc20a8 [Serializer] Revert allowed attributes fix
This was referenced Nov 29, 2023
Merged
Merged
@mtarld mtarld mentioned this pull request Dec 6, 2023
Merged
fabpot added a commit that referenced this pull request Apr 8, 2024
@fabpot
bug #52917 [Serializer] Fix unexpected allowed attributes (mtarld)
49e9184 
This PR was merged into the 5.4 branch.

Discussion
----------

[Serializer] Fix unexpected allowed attributes

| Q             | A
| ------------- | ---
| Branch?       | 5.4
| Bug fix?      | yes
| New feature?  | no
| Deprecations? | no
| Issues        | Fix #52673 #49710
| License       | MIT

A more accurate approach than #52680

Commits
-------

900d034 [Serializer] Fix unexpected allowed attributes
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OskarStark OskarStark OskarStark left review comments

@nicolas-grekas nicolas-grekas nicolas-grekas approved these changes

@dunglas dunglas Awaiting requested review from dunglas dunglas is a code owner

Assignees
No one assigned
Labels
Bug Serializer Status: Reviewed
Projects
None yet
Milestone
5.4
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mtarld @nesl247 @stof @nicolas-grekas @OskarStark @carsonbot