Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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 |
---|---|---|---|
#109 | 1987762-node-add-page-108.patch | 8.6 KB | vijaycs85 |
Comments
Comment #1
damiankloip CreditAttribution: damiankloip commentedI'll grab this one too.
Comment #2
damiankloip CreditAttribution: damiankloip commentedComment #4
damiankloip CreditAttribution: damiankloip commentedAdding the actual services file, and adding the controller to use helper ALOT.
Comment #6
dawehnerDon't you actually need url('node/add' . $type->type, array('absolute' => TRUE)); on there?
removing the hook_menu entry also feels wrong.
Comment #8
dawehnerI guess it's sort of okay to reuse this access checker.
Comment #9
damiankloip CreditAttribution: damiankloip commented#8: drupal-1987762-8.patch queued for re-testing.
Comment #10
InternetDevels CreditAttribution: InternetDevels commented@dawehner Maybe it would be better to move from NodeAddAccessCheck contoller to routing.yml requirements param:
Comment #12
dawehnerGood idea. This works fine here, as the access system uses an OR conjunction at the moment.
Comment #14
damiankloip CreditAttribution: damiankloip commented#12: drupal-1987762-12.patch queued for re-testing.
Meh, seems random.
Comment #16
damiankloip CreditAttribution: damiankloip commented#12: drupal-1987762-12.patch queued for re-testing.
Comment #18
damiankloip CreditAttribution: damiankloip commentedYeah, sorry, that is an actual legit failure.
Comment #19
dawehnerWow, I can't really debug the failure, but I guess once this is a real route this might work out better.
Comment #20
vijaycs85Re-rolling...
Comment #23
damiankloip CreditAttribution: damiankloip commented#20: 1987762-node_add_page-routing-20.patch queued for re-testing.
Comment #25
damiankloip CreditAttribution: damiankloip commentedRerolled, and remove ControllerInterface implementation for NodeAddController. We don't need that if we have no deps atm. Or did you put it there in preparation? even so, if we don't need it yet, let's not use it.
Comment #27
damiankloip CreditAttribution: damiankloip commented#25: 1987762-25.patch queued for re-testing.
Comment #29
damiankloip CreditAttribution: damiankloip commentedRerolled after the RedirectResponse patch.
Comment #30
dawehnerJust wondering whether we should better inject the node add access service/request object?
Comment #32
dawehnerMaybe the failure is related with #2021779: Decouple shortcuts from menu links but honestly no idea.
Comment #33
tim.plunkettStealing this, I have a couple thoughts.
Comment #34
tim.plunkettOkay, injected all the things.
Comment #35
damiankloip CreditAttribution: damiankloip commentedI think we could remove the last of the procedural functions there and use the url generator. Also Since #1979094: Separate create access operation entity access controllers to avoid costly EntityNG instantiation happened, the createAccess just wants the bundle type I think.
Sorry, I had the patch applied, so this seems easier!
Comment #36
dawehnerNice!
Comment #38
damiankloip CreditAttribution: damiankloip commented#35: 1987762-35.patch queued for re-testing.
Comment #40
tim.plunkettMy patch was clearly inferior!
@damiankloip++
Comment #41
damiankloip CreditAttribution: damiankloip commentedHa, I just got lucky this time... :)
Anyway, looks like a schoolboy error in my patch. I guess that's the source of all failures.
Comment #43
damiankloip CreditAttribution: damiankloip commented#41: 1987762-41.patch queued for re-testing.
Comment #45
damiankloip CreditAttribution: damiankloip commentedok, I found #1987760: Convert node_add() to a new style controller which is for the node_add conversion but was closed as a dupe of this. I think it now makes sense to roll what that patch was meant to be into here too, as they will conflict quite a bit anyway, and adding node_add to this patch isn't massive.
Comment #47
damiankloip CreditAttribution: damiankloip commentedWe could extend the NodeAddAccessCheck to check if a node type is present in the request? Then we don;t need another access checker. It would be cool to be able to use _entity_access: 'node.create' but this isn't possible as we wouldn't have a node slug in the path to get upcasted... or can we do some other trickery on the route config like adding 'node: something' or whatever we would need, I doubt that is a. good and b. would work though.
Comment #49
tim.plunkettThe EntityCreateAccessCheck was added specifically because we can't use
_entity_access: 'foo.create'
Comment #50
dawehnerLet's also use the storage controller.
Comment #51
jibranSomehow we can inject this or convert to some helper function in
nodeManger
. This is just a thought. It is out of scope obviously.Comment #52
jibranWhy not
$this->urlGenerator->generate('node_add', array('node_type' => $type))
? Am I missing something?Comment #53
damiankloip CreditAttribution: damiankloip commentedThanks, made those changes. @jibran, sorry I missed your suggestion form the interdiff, but the actual patch has it in. You're totally right, it needs the node_type.
Also we have a bunch of failing forum tests as forum alters the node form for paths like 'node/add/forum/1' etc... by using arg(3) to set a default forum ID. We can't really use arbitrary arguments on the path anymore so I've converted this to use a forum_id parameter in the query string instead. This should fix those tests again.
Comment #54
jibranWhat about creating
nodeTypeManger
and movenode_permissions_get_configured_types
and other helper functions to it? If someone agrees to the idea I can create an issue for this.Comment #55
damiankloip CreditAttribution: damiankloip commented@jibran, having these helpers contained in a class sounds like a good idea to me. It will have to be done at some point anyway. +1
Comment #57
damiankloip CreditAttribution: damiankloip commentedOops, that needed to be $type->type... :?
Comment #58
damiankloip CreditAttribution: damiankloip commentedHopefully this fixes those fails.
Comment #60
jibranHmmm I don't know. Can we use
@inheritdoc
for attributes as well?Let's inject stringTranslationManger as well.
Comment #61
damiankloip CreditAttribution: damiankloip commentedthanks, here are those changes. I think that just leaves the one fail in ShortcutLinksTest, which I'm not sure about the cause of atm.
Comment #63
tim.plunkettDebugging this, the menu_link for node/add is being built wrong:
#2045453: menu_item_route_access() does not catch ResourceNotFoundException helps a bit.
Comment #64
tim.plunkettThis probably isn't 100% related to #2021779: Decouple shortcuts from menu links, but I bet it would fix this.
Comment #65
damiankloip CreditAttribution: damiankloip commented#61: 1987762-61.patch queued for re-testing.
Comment #67
wamilton CreditAttribution: wamilton commentedHere's a trivial reroll with a trivial fix on top for the account vs _account change that was upsetting the node test cases. Fixes some things, probably not all.
Comment #68
becw CreditAttribution: becw commentedThe
_node_add_access
property in the route needed some minor tweaks.[edit: oops, I failed to reload the page before posting this patch]
Comment #70
disasm CreditAttribution: disasm commentedreroll
Comment #71
disasm CreditAttribution: disasm commentedCleaning up the NodeController.
Comment #73
tim.plunkettThis needs to return either static::ALLOW or static::DENY
These are all on ControllerBase, remove them
I'm not sure if this will work since setContainer won't have been called.
Either use create(), put them in protected methods, or just make the calls to entityManager inline.
$this->redirect()
Either inject this like the rest, or don't
use ['#title']
Missing oneliner
Use AccessManager::checkNamedRoute() please
This needs _access_mode: 'ALL'
Comment #74
disasm CreditAttribution: disasm commentedaddressing everything but checkNamedRoute. Couldn't find an example of this being done elsewhere, and too late to dig into it now.
Comment #75
tim.plunkettManually testing helps :)
Comment #76
tim.plunkett#2068287: Support bundle names provided in the request arguments in EntityCreateAccessCheck would help here for NodeAddAccessCheck
Comment #77
damiankloip CreditAttribution: damiankloip commentedNot sure about this change, the node add access checker doesn't care about the values of these?
Comment #78
tim.plunkettOh, I didn't even see that added already. That is the syntax for the issue I just mentioned in #76.
Comment #79
damiankloip CreditAttribution: damiankloip commentedFair, makes sense. thanks!
Comment #81
disasm CreditAttribution: disasm commentedSince we're using code syntax from that patch, here's a combined to see if it improves tests at all. Future developers, DO NOT use this patch for development. Patch in #75 is the correct one.
Comment #83
disasm CreditAttribution: disasm commentedfixing some issues introduced when I converted this to ControllerBase (incorrectly). Manual testing of creating nodes locally works, so this should pass, or at least be close. It may still need #2068287: Support bundle names provided in the request arguments in EntityCreateAccessCheck to pass all tests.
Comment #87
xjmThanks for your work on this issue! Please see #1971384-43: [META] Convert page callbacks to controllers for an update on the routing system conversion process.
Comment #88
disasm CreditAttribution: disasm commentedreroll!
Comment #90
dawehner#88: drupal8.node-module.1987762-88.patch queued for re-testing.
Comment #92
googletorp CreditAttribution: googletorp commentedRerolled the patch, but didn't actually fix anything
Comment #94
andypostuse currentUser() here
seems better to use language manager, related #1966436: Default *content* entity languages are not set for entities created with the API
dot was lost
any reason for?
Comment #95
vijaycs85Thanks for the review @andypost. Just rerolled with fixes for your comments:
#94.1 - Fixed
#94.2 - not sure what we need to change as it is still it progress
#94.3 - Fixed
#94.4 - Not fixed - seems it is changed as part of review(in #73). will leave if for now until hear from @tim.plunkett
Comment #97
damiankloip CreditAttribution: damiankloip commentedredirect() will call generate() so this will not work. You just need to pass route name, and params into redirect.
This is still mixing how the services are used.
Comment #98
damiankloip CreditAttribution: damiankloip commentedAlso, created : #2138073: Remove module_load_include() call from NodeController::revisionOverview()
Comment #99
vijaycs85#97.1 - Fixed
#97.2 - Fixed - updated to use ControllerBase::currentUser() instead of Drupal::currentUser() (thanks to @damiankloip for clarifying it on IRC)
#94.4 - Fixed - looks like that was a unintended change on #73
Comment #101
vijaycs85Seems setting title in add form overriding title set by other places like preview. Removing it and updating title callback to use node_type->name instead of node_type->type.
Comment #102
kim.pepper101: 1987762-node-add-page-101.patch queued for re-testing.
Comment #104
kim.pepperComment #105
kim.pepperRe-roll. Cleaned up a couple of code style issues.
Comment #106
dawehnerIn a perfect world (a follow up) we could typehint for the node type interface.
We should have $this->t() available.
Comment #107
vijaycs85Addressing both issues in #106
Comment #108
andypostThis change is wrong, should be NodeTypeInterface
Comment #109
vijaycs85Updating node_type
Comment #110
dawehnerThank you!
Comment #113
kim.pepperTest failures seem unrelated:
Comment #114
vijaycs85109: 1987762-node-add-page-108.patch queued for re-testing.
Comment #115
andypostback to rtbc
Comment #116
webchickCommitted and pushed to 8.x. Thanks!
Comment #117
xjm(Merging "node system" and "node.module" components for 8.x; disregard.)