Skip to content

fix(serializer): Allow nested denormalization when allow_extra_attributes=false #7270

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

Open
wants to merge 6 commits into
base: 4.1
Choose a base branch
from

Conversation

calbro7
Copy link

@calbro7 calbro7 commented Jul 1, 2025

Q A
Branch? 4.1
Tickets Resolves #6225
License MIT

The linked issue was resolved for JSON LD format with #6451 but not for standard JSON, so this PR implements the same change in the standard ItemNormalizer.

The included test file demonstrates the expected behaviour, it should be possible to alter the Bar entity's someProperty via a PATCH request to edit its parent Foo object.

This test passes with the ItemNormalizer change, but fails without it due to Extra attributes are not allowed (\"id\" is unknown)..

First contribution so let me know if I've done things correctly or if I've missed anything needed for approval!

@calbro7 calbro7 changed the title fix(jsonld): reset gen_id configuration fix: Allow nested denormalization when allow_extra_attributes=false Jul 1, 2025
{
$allowedAttributes = parent::getAllowedAttributes($classOrObject, $context, $attributesAsString);
if (\is_array($allowedAttributes) && ($context['api_denormalize'] ?? false)) {
$allowedAttributes = array_merge($allowedAttributes, ['id']);
Copy link
Member

Choose a reason for hiding this comment

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

what if id isn't the property/attribute used to identify a resource?

Copy link
Author

Choose a reason for hiding this comment

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

Hello, possibly I'm misunderstanding what you mean, I think it always will be id, as it's not configurable is it? And it's also hardcoded eg here:

private function updateObjectToPopulate(array $data, array &$context): void
{
    try {
        $context[self::OBJECT_TO_POPULATE] = $this->iriConverter->getResourceFromIri((string) $data['id'], $context + ['fetch_data' => true]);

When/how could it be something else?

@soyuka
Copy link
Member

soyuka commented Jul 2, 2025

This looks like it breaks one of our test would you be able to identify why? Inside CONTRIBUTING there's everything you need to run tests.

@calbro7
Copy link
Author

calbro7 commented Jul 5, 2025

This looks like it breaks one of our test would you be able to identify why? Inside CONTRIBUTING there's everything you need to run tests.

Hello, thanks for taking a look. Yep I'll get that sorted!

@calbro7 calbro7 force-pushed the fix-issue-6225 branch 4 times, most recently from 52855fa to 2f5e491 Compare July 5, 2025 18:41
@calbro7
Copy link
Author

calbro7 commented Jul 5, 2025

This looks like it breaks one of our test would you be able to identify why? Inside CONTRIBUTING there's everything you need to run tests.

This was because with my changes then there were some scenarios where id shouldn't have been present in the $data passed to the parent denormalizer, but ended up being so.

I've fixed this by unsetting id before that method call (commit), but making sure that it's not unset if it legitimately should be there, to still allow for passing an id when deserializing into a new object (such as the testDenormalizeWithIdAndNoResourceClass test).

Let me know if this approach is good or if I'm overlooking any problems! All the github checks are successful apart from the commit linting, could you advise what's wrong with it because I'm not sure?

@calbro7 calbro7 requested a review from soyuka July 5, 2025 23:44
@calbro7 calbro7 changed the title fix: Allow nested denormalization when allow_extra_attributes=false fix(serializer): Allow nested denormalization when allow_extra_attributes=false Jul 5, 2025
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.

2 participants