Problem/Motivation

If the patch request body doesn't contain ID, then in logs appears this warning:

Warning: Undefined array key "id" in Drupal\jsonapi\Controller\EntityResource->patchIndividual() (line 322 of /var/www/html/web/core/modules/jsonapi/src/Controller/EntityResource.php) #0 /var/www/html/web/core/includes/bootstrap.inc(347): _drupal_error_handler_real() #1 /var/www/html/web/core/modules/jsonapi/src/Controller/EntityResource.php(322): _drupal_error_handler() #2 [internal function]: Drupal\jsonapi\Controller\EntityResource->patchIndividual() #3 /var/www/html/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(123): call_user_func_array() #4 /var/www/html/web/core/lib/Drupal/Core/Render/Renderer.php(580): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\@closure() #5 /var/www/html/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(124): Drupal\Core\Render\Renderer->executeInRenderContext() #6 /var/www/html/web/core/lib/Drupal/Core/EventSubscriber/EarlyRenderingControllerWrapperSubscriber.php(97): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext() #7 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(169): Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\@closure() #8 /var/www/html/vendor/symfony/http-kernel/HttpKernel.php(81): Symfony\Component\HttpKernel\HttpKernel->handleRaw() #9 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/Session.php(58): Symfony\Component\HttpKernel\HttpKernel->handle() #10 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/KernelPreHandle.php(48): Drupal\Core\StackMiddleware\Session->handle() #11 /var/www/html/web/modules/custom/upstart_commerce_blog_management/src/UpStartCommerceBlogManagementBlogMiddleware.php(85): Drupal\Core\StackMiddleware\KernelPreHandle->handle() #12 /var/www/html/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(106): Drupal\upstart_commerce_blog_management\UpStartCommerceBlogManagementBlogMiddleware->handle() #13 /var/www/html/web/core/modules/page_cache/src/StackMiddleware/PageCache.php(85): Drupal\page_cache\StackMiddleware\PageCache->pass() #14 /var/www/html/vendor/asm89/stack-cors/src/Asm89/Stack/Cors.php(60): Drupal\page_cache\StackMiddleware\PageCache->handle() #15 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/ReverseProxyMiddleware.php(48): Asm89\Stack\Cors->handle() #16 /var/www/html/web/core/lib/Drupal/Core/StackMiddleware/NegotiationMiddleware.php(51): Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle() #17 /var/www/html/vendor/stack/builder/src/Stack/StackedHttpKernel.php(23): Drupal\Core\StackMiddleware\NegotiationMiddleware->handle() #18 /var/www/html/web/core/lib/Drupal/Core/DrupalKernel.php(718): Stack\StackedHttpKernel->handle() #19 /var/www/html/web/index.php(19): Drupal\Core\DrupalKernel->handle() #20 @main.

Steps to reproduce

Send a PATCH request to any resource and skip in the body the id attribute.

Proposed resolution

Add isset() check into line 318 of class Drupal\jsonapi\Controller\EntityResource

CommentFileSizeAuthor
#15 3377269-only-test.patch1.93 KBreinfate

Issue fork drupal-3377269

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

_tarik_ created an issue. See original summary.

_tarik_ opened merge request !4484

_tarik_’s picture

_tarik_ commented
Status: Active » Needs review
smustgrave’s picture

smustgrave commented
Version: 9.5.x-dev » 11.x-dev
Assigned: _tarik_ » Unassigned
Status: Needs review » Needs work
Issue tags: +Needs tests

MR should be updated for 11.x

Also will need to research why that value is not set. Could be masking a larger issue.

bbrala’s picture

bbrala
Location Netherlands
commented

Thanks for the contribution, i'd prefer we use 'isset' instead of empty since empty has a few issues.

_tarik_’s picture

_tarik_ commented

Hi smustgrave (#4)
This problem can be reproduced only if the id attribute wasn't added to the request body. So we can be calm about the module functionality.

_tarik_’s picture

_tarik_ commented

Hi bbrala (#5)
Sure I can do this, but could you a bit clarify what issues we can get with using empty() in this part of the code?
I used it because, in this way, we will skip all cases when data['id'] contains an empty string or any other value that can't be for the uuid
Thanks!

project update bot’s picture

project update bot commented

[removed]

bbrala’s picture

bbrala
Location Netherlands
commented

If you use isset then you know the key exists. Although array_key_exists is technically even cleaner, isset is fine. After that the code would check if the value is an uuid. Does that make sense?

_tarik_ opened merge request !4531

_tarik_’s picture

_tarik_ commented

bbrala
Thanks for your response!
I got your point and updated the pull request now it uses !isset() instead empty().
Also was created a new branch for the 11.x version of Drupal core.

_tarik_’s picture

_tarik_ commented
Status: Needs work » Needs review
smustgrave’s picture

smustgrave commented
Status: Needs review » Needs work

Still needs a test

This problem can be reproduced only if the id attribute wasn't added to the request body. So we can be calm about the module functionality.

How is this reproducible from core? This may be a bug in core that putting isset() is covering up.

ReINFaTe made their first commit to this issue’s fork.

reinfate’s picture

reinfate commented
Status: Needs work » Needs review
StatusFileSize
new 3377269-only-test.patch1.93 KB

Added test for the mentioned issue.
Also, there was the same issue just under a few lines that I fixed as well.
I am uploading the test-only patch as well.

This may be a bug in core that putting isset() is covering up.

@smustgrave There is nothing above, Drupal\jsonapi\Controller\EntityResource->patchIndividual() it is Controller. Also, JSON:API correctly responds to the request without the "id" key in the body. The only issue is PHP warnings.

P.S. I realized CI is dead, should I make a separate branch for the test only now?
P.S.S. Or not dead, no idea :)

poker10’s picture

poker10 commented

@ReINFaTe No need to upload a separate test only patch or created a new branch if tests are included in the bugfix patch. We can run a test-only Gitlab pipeline now, which will automatically run the tests from the MR, see: https://www.drupal.org/about/core/blog/drupal-cores-gitlab-ci-testing-is... (Test-only runs are now automated in GitLab CI).

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests +Needs Review Queue Initiative

The tests-only patch does show the issue, so this may be good to go. Lets see.

quietone’s picture

quietone commented

I read the IS and comments. I did not find any unanswered questions.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Reviewed & tested by the community » Needs review

Added a question to the MR.

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Appears the question was answered, doesn't appear a change is required.

  • longwave committed bb6903e8 on 10.2.x 
    Issue #3377269 by _tarik_, ReINFaTe, smustgrave, bbrala: Warning:...

  • longwave committed b930619b on 11.x 
    Issue #3377269 by _tarik_, ReINFaTe, smustgrave, bbrala: Warning:...
longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Version: 11.x-dev » 10.2.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks for answering my question.

This is eligible for backport to 10.2.x as a bug fix.

Committed and pushed b930619b05 to 11.x and bb6903e8e6 to 10.2.x. Thanks!

longwave closed merge request !4484

longwave closed merge request !4531

Status: Fixed » Closed (fixed)

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