Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
ResourceTypeRepository::getFieldMapping is supposed to throw an exception if there is a naming conflict between a generated alias and a fieldname. That check does not work currently since it accesses the wrong variable ($field_name[$alias], but $field_name is the loop variable, a string).
What's more, I think the mapping must be unique in both directions since the ResourceType inverts the mapping (after filtering for string values).
Comment | File | Size | Author |
---|---|---|---|
#10 | 3077661-10-combined.patch | 3.05 KB | cspitzlay |
#10 | 3077661-10-test-only.patch | 1.96 KB | cspitzlay |
#9 | 3077661-9.patch | 3.33 KB | Wim Leers |
#7 | 3077661-7-combined.patch | 3.33 KB | cspitzlay |
#7 | 3077661-7-test-only.patch | 2 KB | cspitzlay |
Comments
Comment #2
cspitzlayHere is a patch that checks against both the existing field names as well as the already generated mapping.
Comment #3
cspitzlayFixed a formatting error.
Comment #4
cspitzlayComment #5
cspitzlayComment #6
Wim LeersWoah! Nice find! 🤯
This will need a failing test to prove it is broken though. This can be added to
\Drupal\Tests\jsonapi\Kernel\ResourceType\ResourceTypeRepositoryTest
. Do you think you could do that? 😊Comment #7
cspitzlayHere is a test that demonstrates the issue.
Comment #9
Wim LeersThis looks perfect! 🤩— see point 3, sorry!Nit: violate 80 cols.
Übernit: missing blank line after last method closing brace.
I spoke too soon 😅 We need explicit test coverage for both of these if-tests.
We can achieve that by not only expecting a
LogicException
but also a particular message, using$this->expectExceptionMessage()
.Comment #10
cspitzlayAfter rereading getFieldMapping I think the conflict in the mapping array cannot happen with the current code.
Thus I could not think of a test for that and I removed the second if.
I still added the check for the message.
I hope I also addressed the nitpicks.
Comment #11
Wim LeersLooks great, thanks! 👍
Comment #14
catchCommitted d9936f2 and pushed to 8.8.x. Thanks!