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.
When adding routes for REST resources, the following line should check for the existence of the index 'supported_auth' before verifying that it is an array:
// Check if there are authentication provider restrictions in the
// configuration and apply them to the route.
if (is_array($enabled_methods[$method]['supported_auth']) && !empty($enabled_methods[$method]['supported_auth'])) {
The order of the conditions should be the other way around (first use empty(), then is_array()) since a resource may define supported_formats but not supported_auth.
This REST configuration will throw PHP warnings:
resources:
'entity:node':
GET:
supported_formats:
- hal_json
The PHP warning is:
A patch will be attached in a comment to address this.
Comments
Comment #1
juampynr CreditAttribution: juampynr commentedHere it is.
Comment #2
clemens.tolboomNice catch.
Comment #3
webchickGood stuff! Is it possible to write a test for this by chance?
Comment #4
clemens.tolboomThe patch fixed a programmers error so I'm not sure what kind of test this needs?
I do think (just a gut feeling) some more tests are needed for REST / HAL but not here.
@webchick I put this on RTBC again but leave 'needs tests'.
Comment #5
webchickRight, this is less about testing for this specific problem (which is highly unlikely to ever come up again), and more to test the specific *code flow* that led to this problem. Because tests weren't already failing, that means whatever steps you took to get this error are missing test coverage, and could also have other bugs lingering. In general, we make testing a "gate" to commit in order to ensure that we fix/add tests when we notice they're missing.
Talked to juampy in IRC and he agreed to add a test here. Thanks, juampy! And thanks for testing REST stuff and making it aweosme, too. :)
Comment #6
clemens.tolboom@juampy do you have some hints to write tests. I want to write those next week.
Comment #7
juampynr CreditAttribution: juampynr commentedSo here are two patches:
* First one contains just the test. The PHP warning I mention above can be seen at admin/reports/dblog once the test has completed. I am not sure we should check for this warning at the log.
* The second one contains both the test and the fix. After running it, there are no warnings in the log.
@clemens.tolboom, feel free to add more tests to this module, but in order to get this fix ASAP, let's deal with them in a different issue.
Comment #8
webchickTest looks good to me at a glance, agreed we shouldn't be checking for this specific error, but rather following the steps you did by hand to make it appear.
Comment #9
clemens.tolboomNice patch ... I never could have done that yet.
I agree with ASAP thus put this on RTBC as test passed.
Comment #10
webchickSounds good. :)
Committed and pushed to 8.x. Thanks!
Comment #11
klausiComment #12.0
(not verified) CreditAttribution: commentedUpdated issue summary.