Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Part of #1971384: [META] Convert page callbacks to controllers
For instructions on how to convert a page callback into a controller, see the WSCCI Conversion Guide.
Comment | File | Size | Author |
---|---|---|---|
#47 | drupal-1987768-47.patch | 13.24 KB | berliner |
#45 | Screen Shot 2013-07-03 at 9.40.41 PM.png | 216.41 KB | piyuesh23 |
#44 | drupal-1987768-44.patch | 11.35 KB | berliner |
#43 | drupal-1987768-43.patch | 9.8 KB | berliner |
#41 | drupal-1987768-41.patch | 9.51 KB | berliner |
Comments
Comment #1
xtfer CreditAttribution: xtfer commentedComment #2
xtfer CreditAttribution: xtfer commentedComment #3
xtfer CreditAttribution: xtfer commentedComment #4
xtfer CreditAttribution: xtfer commentedFirst pass...
I expect this to fail, but I need someone to look at the failing tests and I have to get some sleep, so posting it anyway.
Comment #5
xtfer CreditAttribution: xtfer commentedComment #6
dawehnerSo Node already implements NodeInterface ... so we can already use NodeInterface, right?
If someone has "access content" this will allow access on the node, even node_access fails. I hope we have test coverage for that!
... we need a newline at the end of the file.
Comment #8
xtfer CreditAttribution: xtfer commentedNo, its still using the EntityBCInterface shim. I had NodeInterface here originally.
That's how it works now, is it not?
Anyway, reroll coming...
Comment #9
xtfer CreditAttribution: xtfer commentedThis is blocked by #1987778: Convert node_show() and node_page_view() to a new style controller.
Comment #10
dawehnerThis is how it worked before ..., so access content has not been used before, as far as I see.
Comment #11
monan CreditAttribution: monan commentedThere is a patch posted for #1987778: Convert node_show() and node_page_view() to a new style controller. It's got a typecasting testing problems/issues that I'm not sure how to fix. Any input would be welcome so that it can unblock this...
Thanks!
Comment #12
xtfer CreditAttribution: xtfer commentedYou are correct, not sure how I snuck that in there. Thanks.
I'll try and take a look before Friday, so we can resolve both of them if we can.
Comment #13
vijaycs85Re-rolling...
Comment #15
vijaycs85#13: 1987768-node_view-controller-13.patch queued for re-testing.
Comment #17
dawehnerShould contain the full namespace.
I guess there should be still a title callback for menu links? (not sure tbh.)
Comment #18
vijaycs85Thanks for the review @dawehner.
1. Not sure why we need to have full namespace inside class. As we can use one namespace with that name, we should able to map it in doc generator. I have created #2026085: Remove full path reference of method argument/comment inside class for this.
2. Updated title back.
Comment #20
vijaycs85Seems title callback trying to get the node object from URL which is not happening. So removing title callback and it's function as we are doing same in controller.
Comment #22
vijaycs85#20: 1987768-node_view-controller-20.patch queued for re-testing.
Comment #24
dawehnerThis was easier to fix then expected
Comment #26
pguillard CreditAttribution: pguillard commentedRe-rolled patch
Comment #28
pguillard CreditAttribution: pguillard commented#26: drupal-1987768-26.patch queued for re-testing.
Comment #30
berliner CreditAttribution: berliner commentedAs far as I can see, some tests fail that rely on node_access_test module, e.g.BookTest.php. The problem as I understand it, is that whenever a module implements hook_access_grants() NodeAccessController->accessGrants() is called with the wrong langcode resulting in a query that searches for nodes with langcode 'UND' in the node_access table. The wrong language code comes from the Entity class where the LANGCODE_DEFAULT is hardcoded into the arguments of the entities AccessController instance. If we change that to the actual langcode of the entity then it seems to work.
Besides, it seems only logical to me, that the entities langcode is passed along to the access checker, but then again, I do not completely understand the implications of this.
Comment #31
berliner CreditAttribution: berliner commentedForgot part of the patch.
Comment #33
berliner CreditAttribution: berliner commentedOk, this is obviously too complicated for me. After the last test fail, I just picked the first fail and tracked the error down to the fact, that converted router entries do not have a loading function stored in table menu_router anymore, which effectively breaks node_is_page(). That is because menu_get_object() relies on the existence of load_functions.
Comment #34
berliner CreditAttribution: berliner commentedAnother trial with a proposal from fubhy to temporarily solve the problem with menu_get_object() which effects a lot of other places in the code, e.g. theme.inc, comments, ...
Comment #35
ParisLiakos CreditAttribution: ParisLiakos commentedComment #37
berliner CreditAttribution: berliner commentedSame change to EntityNG access method as in #30:
Comment #39
dawehnerI think we have to keep the title callback/argument for now to support menu links of nodes.
Comment #40
Crell CreditAttribution: Crell commentedThis is a blocker for #2019123: Use the same canonical URI paths as for HTML routes, so bumping to major.
Comment #41
berliner CreditAttribution: berliner commentedComment tests (CommentInterfaceTest.php) for example has failed because the test includes a GET request to /node/NID/COMMENT_ID which results in a 404.
How are fallback callbacks implemented in the new routing system? E.g. before we could do node/3/4 and if no menu item existed for that it stripped the last argument and tried node/3. How is this working in the new system?
Trying another patch, hopefully reducing failures.
Comment #43
berliner CreditAttribution: berliner commentedBreadcrumb test failed because there was no route inheritance for /node/% vs. /node/%/view as it was possible in the old system. I don't know how that can be done using the new routing system, so I added a second route for the moment.
Comment #44
berliner CreditAttribution: berliner commentedFound #1995620: [policy, no patch] Document how to handle routes for MENU_DEFAULT_LOCAL_TASK, so second route is no option. I removed the now obsolete test parts from BreadcrumbTest.php.
Comment #45
piyuesh23 CreditAttribution: piyuesh23 commentedThe patch is working as expected. Tested it for the node/{node} route. Attaching a screenshot for reference.
Comment #47
berliner CreditAttribution: berliner commentedComment #48
berliner CreditAttribution: berliner commentedComment #50
ygerasimov CreditAttribution: ygerasimov commentedShould be "Contains ...\NodeViewController"
After applying this patch, when I open /node/X/view page I got 404. Worst thing is that I still see tabs View and Edit. I do not think it is good, so the approach with having "empty" menu link to "view" tab is not really nice. Can we reuse same route for node/X in node/X/view?
Comment #51
ygerasimov CreditAttribution: ygerasimov commentedTabs to be displayed on 404 page should be fixed in #2004334. Probably breadcrumbs will be better as well. We need to test this patch after applying 2004334.
Comment #52
berliner CreditAttribution: berliner commentedThe 404 for /node/%/view is ok I suppose. This behavior complies with https://drupal.org/node/1953346.
Comment #53
berliner CreditAttribution: berliner commented#47: drupal-1987768-47.patch queued for re-testing.
Comment #55
larowlanRelated #2040265: Add a _entity_view routing default for viewing an entity
Comment #56
Crell CreditAttribution: Crell commentedClaiming.
Comment #57
Crell CreditAttribution: Crell commentedOn further review, I've folded this into #1987778: Convert node_show() and node_page_view() to a new style controller.