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:

Selection_002.png

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

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

juampynr’s picture

Status: Active » Needs review
FileSize
1.19 KB

Here it is.

clemens.tolboom’s picture

Status: Needs review » Reviewed & tested by the community

Nice catch.

webchick’s picture

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?

clemens.tolboom’s picture

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

webchick’s picture

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

clemens.tolboom’s picture

Assigned: Unassigned » clemens.tolboom

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

juampynr’s picture

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.

webchick’s picture

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.

clemens.tolboom’s picture

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.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Sounds good. :)

Committed and pushed to 8.x. Thanks!

klausi’s picture

Component: base system » rest.module

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

Anonymous’s picture

Issue summary: View changes

Updated issue summary.