Problem/Motivation

Follow up to #2853460-25: Simplify RequestHandler: make it no longer ContainerAware, split up ::handle()

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#20 2935783-20.patch6.65 KBWim Leers
#20 interdiff-18-20.txt785 bytesWim Leers
#18 resource-handle-args-2935783.5.patch6.65 KBlarowlan
#18 interdiff-79c41f.txt872 byteslarowlan
#15 resource-handle-args-2935783.4.patch6.61 KBlarowlan
#15 interdiff-c80c09.txt3.2 KBlarowlan
#12 resource-handle-args-2935783-3.patch5.67 KBlarowlan
#12 interdiff-37d43c.txt4.16 KBlarowlan
#10 2935783-10.patch2.74 KBWim Leers
#10 interdiff-8-10.txt751 bytesWim Leers
#8 resource-handle-args-2935783-3.patch2.73 KBlarowlan
#8 interdiff-2d8f20.txt1.49 KBlarowlan
#2 resource-handle-args-2935783.patch1.25 KBlarowlan
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

larowlan created an issue. See original summary.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Active » Needs review
FileSize
resource-handle-args-2935783.patch1.25 KB

I might be making this up

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Issue tags: +Routing

So: you're saying that handle() should have RestResourceConfigInterface $rest_resource_config as one of its parameters, and then the routing system should do the upcasting automatically. Correct?

EDIT: cross-posted — looks like that is what @larowlan meant.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Oh, even without the upcasting!

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented 
+++ b/core/modules/rest/src/RequestHandler.php
@@ -80,14 +80,15 @@ public static function create(ContainerInterface $container) {
+   * @param string $_rest_resource_config
...
+  public function handle(RouteMatchInterface $route_match, Request $request, $_rest_resource_config) {
...
+    $resource_config = $this->resourceStorage->load($_rest_resource_config);

We might need a named slug for this to work, we'll see. Could be dreaming this up.

If it works, would be interested to see if we could make it use the entity converter too by setting the needed options, then we can avoid the ::load

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Unassigned » Wim Leers

Love the thinking, doubt it works, would love to be wrong, will check in the morning!

Status: Needs review » Needs work

The last submitted patch, 2: resource-handle-args-2935783.patch, failed testing. View results

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs work » Needs review
FileSize
interdiff-2d8f20.txt1.49 KB
resource-handle-args-2935783-3.patch2.73 KB

so it works, next step, adding the entity type options and making it use the actual config entity

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Woahhhhh! TIL. 👌

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Wim Leers » jhedstrom
Status: Needs review » Reviewed & tested by the community
FileSize
interdiff-8-10.txt751 bytes
2935783-10.patch2.74 KB

Only fixed a docs nit.

So this doesn't do upcasting, but it does mean there's no more need to inspect the route match's matched route object just to read a route default. Still a win :) One LoC less!

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: jhedstrom » Unassigned

Oops.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Reviewed & tested by the community » Needs review
FileSize
interdiff-37d43c.txt4.16 KB
resource-handle-args-2935783-3.patch5.67 KB

i think we can do better, will need to update the kernel test still (doesn't include #10 yet)

Status: Needs review » Needs work

The last submitted patch, 12: resource-handle-args-2935783-3.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

#12 is what I thought you originally meant, and what I thought that couldn't work.

Looks like it doesn't. 100% of REST integration tests fail with:

Exception: TypeError: Argument 3 passed to Drupal\rest\RequestHandler::handle() must be an instance of Drupal\rest\RestResourceConfigInterface, string given
Drupal\rest\RequestHandler->handle()() (Line: 78)
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs work » Needs review
FileSize
interdiff-c80c09.txt3.2 KB
resource-handle-args-2935783.4.patch6.61 KB

Messed up #12

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented 
+++ b/core/modules/rest/src/Routing/ResourceRoutes.php
@@ -128,7 +128,7 @@ protected function getRoutesForResourceConfig(RestResourceConfigInterface $rest_
-            'type' => $rest_resource_config->getEntityTypeId(),
+            'type' => 'entity:' . $rest_resource_config->getEntityTypeId(),

this was my mistake

Status: Needs review » Needs work

The last submitted patch, 15: resource-handle-args-2935783.4.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs work » Needs review
FileSize
interdiff-79c41f.txt872 bytes
resource-handle-args-2935783.5.patch6.65 KB

winning

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

sweet

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Status: Needs review » Reviewed & tested by the community
FileSize
interdiff-18-20.txt785 bytes
2935783-20.patch6.65 KB

Very happy to be wrong! 😀

And great to have an example in core of how to get a route default to be upcasted!

Only bringing back my nitpick fixes from #10, then this is RTBC.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 2935783-20.patch, failed testing. View results
- codesniffer_fixes.patch Interdiff of automated coding standards fixes only.

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented
Status: Needs work » Reviewed & tested by the community

l.d.o http transfer issues

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented

Indeed! Retesting…

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Just saw this yesterday and thought it looked a bit clunky, then saw this issue today.

Committed/pushed to 8.6.x and cherry-picked to 8.5.x. Thanks!

  • catch committed 096ecb4 on 8.6.x 
    Issue #2935783 by larowlan, Wim Leers: RequestHandler's handle method...

  • catch committed a8b80d8 on 8.5.x 
    Issue #2935783 by larowlan, Wim Leers: RequestHandler's handle method...
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch at Third and Grove commented
Version: 8.6.x-dev » 8.5.x-dev
Status: Reviewed & tested by the community » Fixed

Status: Fixed » Closed (fixed)

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