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:

<?php
// 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:

Selection_002.png

A patch will be attached in a comment to address this.

Files: 
CommentFileSizeAuthor
#7 drupal-php-warning-rest-supported-auth-index-just-test-2076033-7.patch1.27 KBjuampy
FAILED: [[SimpleTest]]: [MySQL] 58,862 pass(es), 0 fail(s), and 3 exception(s).
[ View ]
#7 drupal-php-warning-rest-supported-auth-index-2076033-7.patch2.47 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 59,321 pass(es).
[ View ]
#1 drupal-php-warning-rest-supported-auth-index-2076033-1.patch1.19 KBjuampy
PASSED: [[SimpleTest]]: [MySQL] 58,204 pass(es).
[ View ]
Selection_002.png29.98 KBjuampy

Comments

Status:Active» Needs review
StatusFileSize
new1.19 KB
PASSED: [[SimpleTest]]: [MySQL] 58,204 pass(es).
[ View ]

Here it is.

Status:Needs review» Reviewed & tested by the community

Nice catch.

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs tests

Good stuff! Is it possible to write a test for this by chance?

Status:Needs work» Reviewed & tested by the community

The 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'.

Status:Reviewed & tested by the community» Needs work

Right, 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. :)

Assigned:Unassigned» clemens.tolboom

@juampy do you have some hints to write tests. I want to write those next week.

Assigned:clemens.tolboom» Unassigned
Status:Needs work» Needs review
StatusFileSize
new2.47 KB
PASSED: [[SimpleTest]]: [MySQL] 59,321 pass(es).
[ View ]
new1.27 KB
FAILED: [[SimpleTest]]: [MySQL] 58,862 pass(es), 0 fail(s), and 3 exception(s).
[ View ]

So 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.

Test 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.

Status:Needs review» Reviewed & tested by the community

Nice patch ... I never could have done that yet.

I agree with ASAP thus put this on RTBC as test passed.

Status:Reviewed & tested by the community» Fixed

Sounds good. :)

Committed and pushed to 8.x. Thanks!

Component:base system» rest.module

Automatically closed -- issue fixed for 2 weeks with no activity.

Issue summary:View changes

Updated issue summary.