Updated: Comment #47
Problem/Motivation
Currently, REST module creates all entity routes at /entity/{type}/{id}. That rarely matches the HTML-based route. They should match, to be properly RESTful. Example: JSON representations of nodes should be available at /node/1, not /entity/node/1.
Proposed resolution
Now that NG entities can define their own link relations per #1970360: Entities should define URI templates and standard links, we can just leverage that. To wit:
1) The URI for an entity should be the "canonical" link the entity defines.
2) We have to create a new link relation to specify the URL path where to create new entities. The current proposal for the name is "http:/drupal.org/link-relations/create", a fully qualified domain name. Example: /node would be the link for POSTing a new node. This is the most reasonable thing we can do for now until we settle on a solution how we deal with link relations in general. This is discussed in #2113345: Define a mechanism for custom link relationships and does not block this issue.
3) We have to split up MimeTypeMatcher into AcceptHeaderMatcher and ContentTypeHeaderMatcher in order to be able to differentiate properly between POST requests to the same path that carry form data vs. JSON or other formats. Example: POST to /node with Content-type: application/hal+json should not end up on some node/view HTML route.
4) We have to initialize the _method restriction on routes to GET|POST so that other routes with different HTTP methods have a chance to be matched. Example: a DELETE request to /node/1 should not end up on the usual node view HTML route.
Remaining tasks
* Commit the RTBC patch.
User interface changes
None.
API changes
Entity URIs for web services change to what they should be anyway. Example: /entity/user/1 changes to /user/1.
Related Issues
#1970360: Entities should define URI templates and standard links
#2113345: Define a mechanism for custom link relationships
Steps to make this manually testable
There is lots of documentation on all the REST stuff here: https://drupal.org/documentation/modules/rest
Here's the quick way to get you started on making REST requests:
- Enable modules rest and hal
- Set permission "Access GET on Content resource" for anonymous users.
- Get a REST Client (there's for example a firefox called RESTClient)
- Create a node for testing
- Make /node/$nid GET request with the "Accept: application/hal+json" set
Change Record
[#2096019]
Comment | File | Size | Author |
---|---|---|---|
#122 | interdiff.txt | 955 bytes | catch |
#119 | 2019123.patch | 57.18 KB | klausi |
#88 | 2019123.patch | 55.62 KB | klausi |
#81 | 2019123.patch | 56.5 KB | Github sync |
#74 | 2019123.patch | 58.14 KB | Github sync |
Comments
Comment #1
Crell CreditAttribution: Crell commentedComment #2
Crell CreditAttribution: Crell commentedComment #3
Crell CreditAttribution: Crell commentedHere's a first stab. It's nowhere close to done, and tests are failing locally without giving me any useful direction, but let's see what testbot says.
I'm not convinced that create-form is the right link to use here, but I couldn't find anything better.
Comment #5
Crell CreditAttribution: Crell commentedDiscussed a bit on the REST call today. create-form is not the right link. IANA doesn't have a right link yet.
Crell suggested that "drupal:create" might be technically a valid complete URI per
http://www.ietf.org/rfc/rfc2396.txt
But we should verify that. We want something that is "semantically correct, standards correct, and quick to implement." Whatever that is.
Comment #6
klausiHere is an attempt that is nowhere near ready, but shows the approach I want to take:
* "link" can be defined on the REST resource annotation
* the entity derivative copies the link patterns from the entity info to the resource plugin info.
* the generic routes() method checks if such links pattern exist on the plugin and uses that if available, with a fallback to just using the plugin name in the route.
* I'm using "drupal:create" as a link name for now until a better suggestion is made.
Problems:
* This completely breaks node paths for example because our content negotiation sucks (visiting /node/1 in firefox returns REST module's XML response instead of the node page). :-(
* I don't know if this extraction of named request parameters is correct:
Comment #7
ygerasimov CreditAttribution: ygerasimov commentedLets have example of resource annotations to have "create" link.I would propose to have "rest:create" instead of "drupal:create" as a link name as "rest:create" more intuitive (at least for me).
Comment #8
klausiTried to experiment with MimeTypeMatcher, because I thought we can filter out less fitting routes there so that requests to /node/1 work again in web browsers. Turns out that the filter() method only retrieves 3 REST routes, so the usual HTML page route is missing. Maybe the /node/{node} route is still in the old menu system?
Have to give up for today.
Comment #9
ygerasimov CreditAttribution: ygerasimov commentedLooking at node.module node_menu() I see that '/node/%node' is still in old menu system.
Comment #10
linclark CreditAttribution: linclark commentedAs noted on the REST call yesterday, using a CURIE style (e.g. drupal:create) is probably not what we want here.
As the CURIE spec says:
A CURIE is only interpreted as a URI if the host language supports CURIEs. We don't currently define a namespace mapping for the prefix "drupal" anywhere, so even if it were being output in a host language that does support CURIEs, it would be an invalid CURIE.
Someone on the call yesterday suggested that this is still a valid URI even if we don't consider it a CURIE. Here is the structure of URIs.
In this case, we would be defining a whole new scheme, either called "drupal" or "rest". This means we are not using the HTTP scheme. I can't see a reason why we would want to do this.
Comment #11
klausiOk, so working with nodes is blocked by #1987768: Convert node_page_view() to a new style controller. I guess there a lot of other WSCCI conversion issues affecting entity paths that are not yet resolved.
Even entity test still uses hook_menu(). So we have no way to make any progress here unless we have at least one entity type that is NG and uses the new routing system.
Comment #12
ygerasimov CreditAttribution: ygerasimov commentedI have played with idea of adding a legacy route in RouteProvider before we get to MimeTypeMatcher and it gets into endless recursion (I have added static flag to prevent it). Now if you call node/1 with browser you get html, and when you call with header Accept: application/xml you will get xml. But I am not a big fan of this solution. Hope we can get it less ugly.
Comment #14
klausi@ygerasimov: that hack is not so bad, but it breaks all other pages with endless loops.
I have no idea how the current legacy menu system integration into the routing system works. Where is LegacyUrlMatcher called from? If we have Symfony routes for a path does that mean that the legacy routes can never be called? Where is the distinction between legacy and new routing system done? In DynamicRouter, NestedMatcher, ChainRouter, some event subscriber or in the kernel?
Is the approach of inventing an artificial route object that we generate from the old menu system feasible?
At this point I'm a bit frustrated that I could not answer that questions myself after 2 hours of interactive debugging. Drupal 8 is just a bit too complex for me now.
Unassigning from myself, I'm stuck at developing this further. Feedback welcome!
Comment #15
klausiSo this is probably not going to happen for code freeze, but there might be another way out: we keep the /entity/{entity_type}/{entity} paths and just add the canonical entity paths as well later. So we could deprecate the old paths but still keep them for client compatibility.
Technically this would be an API addition, which is also not supposed to happen after code freeze (I think). Maybe we can get an exception here, since it does not break anything but rather solves a big WTF.
Going with that approach we can avoid any struggle with the old menu system and do this later once most of the HTML entity paths are converted.
Comment #16
linclark CreditAttribution: linclark commentedscor tells me that API additions are still possible after API freeze, since it is no longer a code freeze. I think that they are being careful about what gets in, though.
I think it will be a real shame if we can't get the paths to align between HTML and other serializations. Just yesterday, someone here at DevDays was testing REST and set the path for node/1 to entity/node/1 in the form because they thought that's what they were supposed to do.
Comment #17
linclark CreditAttribution: linclark commentedEDIT: posted twice.
Comment #18
Crell CreditAttribution: Crell commentedAPI change or not, I'd argue that not having consistent paths is a straight up bug. We need to fix that bug before launch, ASAP.
Klausi: The routing system entry point is ChainRouter. That calls the legacy system and then DynamicRouter in turn. LegacyMatcher is part of the old system. If LegacyMatcher doesn't find a route then DynamicRouter is called. DynamicRouter contains the NestedMatcher, which is what looks up routes from the DB, filters them, etc.
It's probably not feasible to create route objects from legacy routes, especially since we're planning to remove legacy route support entirely. However, if a route comes from the legacy matcher then there will be a _legacy attribute set on the route, set to true. We if-else on that in a couple of places for now; that's the easiest way to deal with that problem for the moment.
Comment #19
webchickThe only BC break here seems to be just that URIs are changing; I don't think this needs approval, go for it. But if there are other module developer-facing changes required to get this done, we need that info in the issue summary.
Comment #20
Crell CreditAttribution: Crell commentedI will take that as approval. :-)
This is still blocked by #1987778: Convert node_show() and node_page_view() to a new style controller at the moment.
Comment #21
Crell CreditAttribution: Crell commentedThat issue is no longer blocking, thanks to #2089635: Convert non-test non-form page callbacks to routes and controllers. So we can move forward here.
Comment #22
klausiThe good news: this now basically works for me for nodes during manual testing.
The bad news:
* lot's of REST tests need to be fixed.
* we return now 200 with HTML for invalid formats on nodes instead of 406, is that ok with us?
* i'm not sure about the algorithm in the MIME type matcher and whether we should do some optimization for HTML routes and routes that don't specify any format requirement.
* Will still haven't settled on the naming of the link relation for creating entities with POST, still using drupal:create in the patch.
Comment #24
klausiThe problem is that nodes and entity_test behave differerntly on non existing entities. A non existing node path returns an Exception from upcasting which results in an obscure plain text response body. On the other hand entity_test is NOT upcasted and the error is handled by the resource plugin.
Not much progress.
Comment #26
klausiNot much luck with this.
* Drupal AJAX is weird. I tried to mark ajax route as "_format: drupal_ajax" so that they match with the Accept headers of AJAX requests, but that did not work because then the request object suddenly does not know about the drupal_ajax format anymore. Anyway, I added a similar workaround as in the content negotiation class to just list those formats as default.
* PATCH requests for nodes do not work. The problem is that the two routes node.view and rest.entity.node.PATCH match the request. The MIME type matcher only acts on Accept headers (a PATCH request carries the format in the Content-type header), so both routes survive the matching. Then the first route is picked randomly, which is the node.view route and the usual node GET controller is executed.
I see two possibilities to solve this:
* Make MimeTypeMatcher also look for the Content-Type header on write requests or create a new matcher that targets this. Not sure if we should also use the _format requirement on routes or a second _content_type_format?
* Make UrlMatcher better at filtering out routes per HTTP method. We have explicitly specified PATCH as required method on the REST route, should the UrlMatcher throw out other routes that do not specify that exact HTTP method? Actually it should not throw out the route but rather adjust the fitness of the routes.
Comment #28
andypostRelated #2102129: Map entity operations to route names
Comment #29
klausiThe route selection problem even more applies to POST requests. Let's assume I have a standard HTML route with a form on it on /node:
Then this will collide with the REST route:
So the routing system will never be able to select the second REST route, because it does not look at the Content-type HTTP header. The only way to differentiate between two POST requests is to examine the incoming data: one request uses application/x-www-form-urlencoded to submit a HTML form and should be mapped to the first route, the other uses application/hal+json and should be mapped to the second route.
Looks like we need a HTTP Content-type MIME type matcher and a new requirements property "_content_type_format" for routes. Should we mix that into MimeTypeMatcher, so that it matches based on Accept and Content-Type headers? Or should we create a new ContentTypeMatcher class?
I'm going to experiment with an advanced MimeTypeMatcher class.
Comment #30
klausiWhat we decided on the last REST call:
* DELETE routing collision with existing node paths that return HTML. Conclusion: default all default HTML routes to GET|POST in their _method requirement, use a route alter event for that.
* Same for POST requests. Conclusion: add a HTTP content type matcher and rename MimeTypeMatcher.
So this patch now fixes the problem with DELETE requests on the /node/1 path never matching the REST routes by adding a RouteMethodSubscriber.
The interdiff is not fully complete and this will still fail on the testbot. Coming up next: Creating ContentTypeMatcher that will act on non GET requests.
Comment #32
klausiUpdated patch, including the split-up of MimeTypeMatcher to AcceptHeaderMatcher and ContentTypeMatcher.
This should get us pretty far I hope, let's see what test fails remain.
TODO:
* expand unit test for AcceptHeaderMatcher
* create unit test for ContentTypeMatcher
* create REST test for GET JSON /node/1
Comment #34
klausiSo we simply cannot remove routes in the matcher if they don't have any restrictions, because then all the things break. New approach: prioritize the returned route collection with better fitting routes on top of the list and generic routes moved down. This has the disadvantage that we rely on internal implementation details of the RouteCollection class (it never says anything about order in the collection), but it should cover our use case.
Comment #35
klausiYay, so the tests pass now!
This is now ready for review. I added phpunit test cases for both new matcher classes.
We still have a drupal:create link relation in there, which is not a standards-compliant name. Other suggestions?
Comment #35.0
klausiAdding docs to test this manually to the summary.
Comment #36
klausiUpdated the issue summary.
Comment #37
dawehnerTo be honest it is a bit odd to add default values, as this increases the site of every route, even we actually could just apply the default logic on the RouteFilter level.
We could just use String::format
Just wondering how it is possible that the entity is not upcasted yet. Can't we fix that on the param converter level? The amount of workarounds seems too much.
I try to understand the different of _route_params and _raw_variables which should have the same information.
Can we just go with $key[0] !== '_' this describes it a bit easier what is going on.
We could use the new object instead but this is out of scope;
We could use @expectedException with this change.
If we create such separate tests just use multiple descriptive test methods.
Just use @expectedException as annotation. I don't care whether the other example does it different.
Comment #38
Crell CreditAttribution: Crell commentedThat introduces more overhead at runtime along the critical path. This is a compile-time operation. I am really not concerned about the size of the serialized route object; certainly not for adding, what, 8 bytes?
Comment #39
klausiExcellent review dawehner, thanks!
Now that I think about it we can just use the entity upcaster/enhancer for all our routes, then we don't have to deal with two cases which simplifies the code a lot.
I agree with Crell that the compile time approach looks better to me.
I tried to address the rest of your comments in this patch. No idea about _route_params vs. _raw_variables, so I left that unchanged.
Comment #40
dawehnerNice! I did not know what you can also check the actual message.
I am wondering whether the concept of canonical links etc. really belongs into the ResourceBase or rather is coupled to entities.
Comment #41
klausiYes, it belongs into resource base because we not only want to support link definitions on the entity derivative plugin but also on other resource plugins. So people could use a links annotation on their resource plugin class (which basically looks the same as on an entity class), which is cool.
This updated patch now replaces the drupal:create link relation name with the fully qualified name http://drupal.org/link-relations/create . A page on drupal.org does not exist yet, but that is not a requirement. It could be used later for documentation purposes (explaining the meaning of this link relation in Drupal land).
Comment #42
Crell CreditAttribution: Crell commentedIsn't there a more specific 4xx error in this case? This looks like a 415 to me:
http://httpstatus.es/415
(I think there is a Symfony exception for that, which we were previously using in place of 406, because those two are confusingly similar.)
Routing is priority 32. We don't need to have priorities of a bajillion. Anything larger than 32 is fine. (Why is it 32? I have no idea. I borrowed that from fullstack.)
This is a micro-op. It's also a sloppy one. It's 2 lines of code. Duplicate it. :-)
Comment #43
linclark CreditAttribution: linclark commentedThis might change based on #2113345: Define a mechanism for custom link relationships. If we don't use link relations for the keys, then we would likely change this key to something a little more readable.
Comment #44
star-szrComment #45
klausiHow could I miss 415? Yep, that is exactly what we need. Also fixed the other stuff.
Ignoring the link relation discussion, we can change that later if we want.
Comment #46
dawehnerChange interdiff looks good. The patch adds good testcoverage etc.
Comment #46.0
dawehnerExpanded on proposed solution.
Comment #46.1
klausiclarified link relation usage, remove part about rendering links in entities
Comment #47
klausiUpdated the issue summary.
Comment #48
pwolanin CreditAttribution: pwolanin commentedDoes this leave in place the 2x definition of the path in the route and the annotation? we need to come up with a plan to fix that if it's not fixed here.
Comment #49
klausiThis is not fixed here, that would be a separate issue.
Comment #50
catchSorry it took me a while to get to this. Could we do some profiling of the route negotiation on nodes? This looks like it could impact that and it'd be good to know if it does.
Comment #51
Crell CreditAttribution: Crell commentedWhat sort of profiling do you see needed?
Comment #52
catchThe inbound processing for node/n. For example we'll be doing that if a node is in a breadcrumb - so say there were six breadcrumbs all pointing to nodes (via aliases), does this add any overhead or not?
Comment #52.0
catchfixed numbering
Comment #53
catchCNR for the profiling.
Comment #54
klausiSo I tried to do some profiling with 8 nodes and 8 breadcrumbs looking at the PathBasedBreadcrumbBuilder.
I had a look at it with xhprof at the most outer node page that has 8 breadcrumbs on it.
Drupal 8.x:
This patch:
So the whole page request was a bit slower with this patch, which was expected. 4 milli seconds more have been spent in the nested routing matcher where the new matchers from this patch are invoked. If we only look at this part the performance impact is about 6% (from 67ms to 71ms). If we look at the whole page request then it is roughly 2% (from 539ms to 550ms).
It might also be interesting to look at the case when more routes are available at the node paths with REST module:
Drupal 8.x with basic_auth, HAL, REST and serialization enabled:
This patch with basic_auth, HAL, REST and serialization enabled:
So this has a bigger impact, because we have one route at /node/{node} in D8 right now and six routes with this patch. Interestingly the overall page request time only increases 1ms, but that number is volatile anyway as the whole page request time varies +-50ms on my computer. The time spent in the nested matcher increases 31% from 69ms to 91ms.
So these numbers look like they cannot be trusted because they are volatile. One xhprof run just does not cut it.
I wrote a script to generate more numbers to get to a useful median value. It invokes the system.breadcrumb.default service 100 times and measures the first run (more expensive building all the objects) and the subsequent runs average. I ran that script more than 20 times per test case and collected the numbers in a spreadsheet.
So we have some notable performance impacts here when we benchmark directly at the breadcrumb build service. If you look at the raw data you can see that the data points vary a lot, so there is still some fuzzy margin of error around this test. Please also note that we are testing an edge case here with many breadcrumbs that needs to run request matching many times.
I think we can conclude that we are making Drupal a bit slower with this patch, ranging from 0 to 14ms on my computer. That translates into 0-2.5% more in overall page request time, depending how often the whole request matching pipeline is executed.
Given that we adopted the new routing system because of its design and flexibility we should also make proper use of it, which we try to accomplish in this patch. We buy that with a small performance penalty, but my testing did not show any serious slow down.
More testing or do we want to go back to RTBC?
Comment #55
klausiAdded profiling data and script.
Comment #56
Crell CreditAttribution: Crell commentedCatch, is that acceptable? If not, we'd need suggestions on how else to handle it because HEAD is currently incorrect in this regard by not using /node/{node} for REST module.
Comment #57
catch0-2.5% of total request time is only such a small percentage because overall request time is so bad at the moment.
If it's around 14ms then that's closer to 10% of Drupal 7 request time for at least some pages, which is a significant regression.
Would be good to see a bit more from inside matchRequest() to see what's taking the extra time. An xhprof diff report would probably be most useful. If we can optimize the lookup that'd be the best thing - this is really the first time it's been applied to anything that's used a lot.
Comment #58
Crell CreditAttribution: Crell commentedThis is only an API change for REST clients, of which there are none at present. However, I do consider fixing this to be a necessary, release-blocking API change and thus beta blocking.
Comment #59
klausiRerolled patch.
Added 2 performance optimizations:
1) Don't create new RouteCollection objects in AcceptHeaderMatcher and rather manipulate the incoming object.
2) Restrict the fake request object in PathBasedBreadcrumbBuilder to only one accepted format (text/html) to reduce matching costs in AcceptHeaderMatcher.
I also did some XHprof profiling for Symfony\Cmf\Component\Routing\NestedMatcher\NestedMatcher::matchRequest(), will follow-up with that later.
Comment #61
klausiEdit: Moved profiling table to http://klau.si/sites/default/files/profiling.html
So this is pretty bad. Drupal has to match 18 requests on the frontpage (logged in as admin), WTF? Of course that cumulates to a loss of 4ms overall (up from 39ms to 43ms on the invocation of NestedMatcher::matchRequest()).
Honestly I don't see any hope for this patch to keep up with performance while we have to match so many fake sub-requests.
So should we close this as won't fix or has anyone ideas how to reduce the number of matched fake requests?
Comment #62
andypostProbably related #1839516: Introduce entity operation providers
Comment #63
effulgentsia CreditAttribution: effulgentsia commentedI do, but too tired to write it up right now. Will do so tomorrow.
Comment #64
effulgentsia CreditAttribution: effulgentsia commentedWell, turns out the direction I was thinking is made obsolete by #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit, which fixes it a different way. With that patch, the number of calls to matchRequest() for an admin viewing the front page goes down to 3. That's still 2 too many, but that's a separate issue (#2150833: Performance regression in menu access checks). I suspect if you benchmark the impact of this patch after #2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit lands, it'll turn out to be acceptable, and will get even better as the remaining occurrences of #2150833: Performance regression in menu access checks are fixed.
Given that it's insane for us to call matchRequest() for anything other than the one actual HTTP request being handled (not counting non-performance-critical steps, like saving a menu link or rebuilding menu caches) my recommendation is to allow this in, and fix those needless invocations in those other issues. But, that's a call for catch to make, not me.
From catch in #52:
#2047633: Move definition of menu links to hook_menu_link_defaults(), decouple key name from path, and make 'parent' explicit decouples breadcrumbs / menu parents from paths (or if it doesn't fully, then there should be a follow up to finish that), so no longer an issue, right?
Comment #65
andypostthe same applies for taxonomy terms and other entities that could have limited access on per user basis
Comment #66
klausiLooks like we can continue here, since the performance regression will be dealt with in other issues.
This approach will not work until we have URI templates back in entity link annotations, postponing this on #2150747: Switch back to URI templates in entity annotations.
Comment #67
xjmRelease blockers are by definition critical, so bumping accordingly.
@catch pointed that if this is only a change to the REST module API, it's not a beta blocker. Can we either move this issue to the entity or routing system as appropriate, or remove the beta blocker tag?
Comment #68
Crell CreditAttribution: Crell commentedTo clarify, it's an API change for "people talking to a Drupal 8 site over non-HTML", which just so happens to be provided by rest.module. It's not an API change for developers interacting with rest.module. (It may be, but that's less of an issue, I agree.) Changing the REST API of Drupal is rather the definition of "API break". :-)
Comment #69
catchYes but changing that doesn't block a beta IMO - people creating a REST client for 8.x are not the target of beta, module developers are.
Comment #70
Crell CreditAttribution: Crell commentedI find the idea that those are necessarily different people odd, but I'll compromise here. (Critical = 8.0.0 blocking, but not beta blocking. Changing the path after 8.0 would definitely be a problem.)
Comment #71
catchIt's a quantitative vs. qualitative difference.
Comment #72
Github sync CreditAttribution: Github sync commentedklausi opened a new pull request for this issue.
Comment #73
klausiUn-postponing this: maybe we can make this work with route names in entity annotations.
So what I'm doing now is to extract the path patterns from the routes when we build the entity REST plugins in EntityDerivative.php.
This is now rather inconsistent, because in entity annotations we have route names for links and in REST plugin annotations we have path patterns for links, but I want to see how many test cases fail first.
Comment #74
Github sync CreditAttribution: Github sync commentedklausi pushed some commits to the pull request.
Comment #75
klausiSo I renamed "links" to "uri_paths" in the REST plugin definition to better distinguish it from "links" in entity annotations. That should make it more clear that we are dealing with URI patterns here nd not with route names as in the entity links annotation.
Comment #77
XanoCan you please provide interdiffs?
Comment #78
klausiSure, just look at the latest commits of the pull request:
https://github.com/klausi/drupal/commit/ed84fe5d56ff933d29d2d05869dd8633...
https://github.com/klausi/drupal/commit/9a79b743d103a7ad7589a8e516263614...
Comment #79
klausi74: 2019123.patch queued for re-testing.
Comment #81
Github sync CreditAttribution: Github sync commentedklausi pushed some commits to the pull request.
Comment #83
klausi81: 2019123.patch queued for re-testing.
Comment #85
klausi81: 2019123.patch queued for re-testing.
Comment #86
klausiThose test fails look very random; the first was an out of memory error, now something unrelated UserCancelTest. Retesting one more time.
Comment #87
dawehnerGreat work! The issue title is a bit of vague and confused me.
Out of scope of this issue.
For now we maybe should set a higher priority.
It would be great to also explain why it does not make sense
Note: We can also use $request->isMethod('GET') instead.
It feels a bit odd to throw an exception here, but i cannot think of a better place.
As general new issue we could think about having a separate access method. Not a problem at all for this issue.
Could we use the controller resolver instead to figure out the needed parameters?
One thing we do is using @coversClassDefault \Drupal\... to help phpunit code coverage.
Let's interhitdoc
These 3 ones have really similar structure so we could convert them to use a @dataProvider
Comment #88
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #89
klausi@dawehner: RouteFilterInterface specifies that an exception should be thrown when all routes are filtered out by a matcher, that's why we throw an exception.
I don't think we can use ControllerResolver, because the needed parameters are dynamic. The resource plugins will handle the parameters, so maybe we could get parameter information from them with reflection, but I would like to keep it simple for this patch for now.
Addressed the other comments, thanks!
Comment #90
dawehnerSadly we need to create a change notice now.
Comment #91
klausiChange record draft created: https://drupal.org/node/2199185
Comment #92
dawehnerI am fine with this now, though klausi, seriously, please don't make the life of reviewers harder than it has to be and upload an easy to access interdiff manually.
Comment #93
klausi88: 2019123.patch queued for re-testing.
Comment #94
webchickOk, this has sat here at RTBC for quite awhile, so I took a peek. I wasn't expecting an issue with such a nice, simple change notice to involve 55K worth of changes. :) Though a lot of them are in tests.
I don't follow everything that's going on in here exactly, but the changes are salient and well-commented and tested. When I pinged Crell about it though he said he had some earlier architectural concerns and wanted to take an eyeball first. Assigning to him.
Comment #95
webchickMoving out of the RTBC queue to reflect that.
Comment #96
Crell CreditAttribution: Crell commentedThis looks reasonable to me overall. I only have one comment worth mentioning, and it's for the tests:
If we have the $entity, why are we forming the URI manually? Just ask the $entity for it, no?
(That appears a few dozen times in the tests.)
Not RTBCing yet pending answer to the above.
Comment #97
tim.plunkett$entity->url() ftw
Comment #98
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #99
klausiNow using the entity url() method in the tests as proposed!
Comment #101
tim.plunkettPlease upload an interdiff so we can see what broke between this and #88.
Comment #102
klausiSure, here you go: https://github.com/klausi/drupal/commit/62a260d91fc9318967872ea5fede4d94...
Comment #103
klausi98: 2019123.patch queued for re-testing.
Comment #105
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #107
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #109
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #110
klausiVery good, this now finally passes the testbot again.
1) We have to use $entit->getSystemPath() instead of ->url() because we are generating the full URL later. Interdiff: https://github.com/klausi/drupal/commit/306aaf32677c52da405a647b4b904348...
2) InstallUninstallTest catches some interesting corner cases when entity routes might not be available during plugin derivative building. That's why I'm catching RouteNotFoundException and just exclude the incomplete entity type during module install/uninstall phases. It is ugly and you won't like it, but it is robust. Better suggestions welcome: Interdiff: https://github.com/klausi/drupal/commit/84e40f8e87900ec22e60af88f1add739...
Comment #111
Crell CreditAttribution: Crell commentedComment #112
dawehner+1
Comment #113
catchDoesn't this mean the plugin derivates are built before the routes? If it's consistent, then changing the order of those ought to be enough no?
Comment #114
klausiThe derivatives have to be built before routes, because during route building those derivatives are invoked. So they will have to deal with outdated route information, because the new routing information is just being built. It's a cyclic dependency: we need existing routes while we are building routes, so that circle will break somewhere and we are catching that here.
I think we cannot change that order, what would you suggest?
Comment #115
dawehner@klausi
This really reminds me of the discussion on #2158571: Routes added in RouteSubscribers cannot be altered
Comment #116
Crell CreditAttribution: Crell commentedIf #2150747: Switch back to URI templates in entity annotations happens (which I still want it to) that code will become irrelevant anyway. Let's not block this issue on #2158571: Routes added in RouteSubscribers cannot be altered since it's not moving right now. If we can do better on that bit of code later it's not an API change, whereas moving URLs around IS an API change and needs to happen.
Comment #117
webchickBack to catch.
Comment #119
klausiklausi pushed some commits to the pull request.
For an interdiff please see the list of recent commits.
Comment #120
klausiSimple reroll, fixed the merge conflict, back to RTBC.
Interdiff: https://github.com/klausi/drupal/commit/64d0afdabf437bbd49813fbead9cf343...
Comment #122
catchOK I added a @todo for that issue locally and committed/pushed this one. Interdiff attached.
Comment #124
yanniboi CreditAttribution: yanniboi commentedUpdating documentation for this change.
Comment #125
yanniboi CreditAttribution: yanniboi commentedRather than creating a change record, I updated the documentation page here: https://drupal.org/node/2096019
Comment #126
klausiThanks! Change record already exists, linked above.