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.
Committers, a few additional people need commit credit on this patch because #1987768: Convert node_page_view() to a new style controller was merged into this issue (see #62):
berliner, pguillard, vijaycs85
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 |
---|---|---|---|
#105 | node_show-1987778-105.patch | 11.48 KB | dawehner |
#105 | interdiff.txt | 2.71 KB | dawehner |
#99 | 1987778-99.patch | 10.05 KB | damiankloip |
#99 | interdiff-1987778-99.txt | 742 bytes | damiankloip |
#95 | 1987778-95.patch | 10.11 KB | damiankloip |
Comments
Comment #1
monan CreditAttribution: monan commentedCurrently trying to work on this.
Here is a node that talks about adding a custom access controller: http://drupal.org/node/1851490
Slow going... Getting familiar with things...
Comment #2
monan CreditAttribution: monan commentedShould node_show() be refactored out of the node.module and into the controller?
Comment #3
monan CreditAttribution: monan commentedHere is a patch for this issue. An open question: I'm getting watchdog 'non-fatal' errors on the typing of the parameters - not sure how to fix this. Tried converting in the routing yml file to no avail.
Also should be noted that this issue is blocking issue #1987768: Convert node_page_view() to a new style controller.
Guessing the controller's node_show function should be moved to a service so that we can call it from other places in the module.
Comment #5
monan CreditAttribution: monan commentedI know this failed, but I don't know how to fix the issue, as I reported in my previous comment.
Any thoughts?
Comment #6
xtfer CreditAttribution: xtfer commentedNodeInterface doesnt have a
use
statement at the top of the file so it's import fails, but it will fail anyway, as Node hasnt been converted yet. EntityInterface works, and is probably preferable at this point.Integer isn't a valid type hint. PHP thinks you are hinting a class called "\Drupal\node\Access\integer".
Those should fix your type problems, I hope.
Use EntityManager for this.
And this.
This should use:
drupal_container()->get('module_handler')->moduleExists('history');
instead.
You can specify that the args should be numeric by using:
defaults:
node: \d+
node_revision: \d+
This will stop this router from triggering on paths like node/add/revisions/...
Also, to get the patches into a committable state you will need to clean up the whitespace, comments, etc.
Comment #7
xtfer CreditAttribution: xtfer commentedComment #8
rbayliss CreditAttribution: rbayliss commentedI think you mean \Drupal::moduleHandler()->moduleExists('history'). Or, better yet, inject the module handler directly.
Comment #9
xtfer CreditAttribution: xtfer commentedYes, I mean that. Thanks.
Comment #10
monan CreditAttribution: monan commentedOk, here is a fixed patch. I think issues are resolved, but we'll see what testbot says..
Thanks xtfer and rbayliss for the direction!
Comment #11
xtfer CreditAttribution: xtfer commentedTestbot seems to be struggling...
A manual test of the functionality seems to work, however the patches need a bit of a coding standards clean-up. Mostly its ensuring line's wrap at the correct width and that all the functions are documented correctly. Some of the inline comments could also be checked for accuracy, as they are a bit out of date now.
Some selective points...
Needs a @file docblock
Some methods in this class need a one-line summary of what they do as well as the description.
Apparently global $user is sufficient here, but it also seems to be duplicated further down...
One space after "if"
Rogue PHP closing tag. Remove it.
This class needs docblocks for the missing methods. They can just use {@inheritdoc} if they match the parent.
Comment #12
monan CreditAttribution: monan commentedThanks... Clearly need to brush up on my Drupal coding standards...
I'll put a final one together and submit again...
Comment #13
monan CreditAttribution: monan commentedOk, hit all the points that xtfer made, cleaned up docs, fixed line wrapping where appropriate according to coding-standards. Hopefully this will be the final final.
Comment #14
dawehnerBoth long function blocks could just use {@inheritdoc} so you don't have to copy all of the docs.
Please add a newline at the end of the file.
This should be "Contains \Drupal\..."
Let's also inject moduleExists in there.
Comment #15
xtfer CreditAttribution: xtfer commentedComment #16
monan CreditAttribution: monan commentedHi dawehner, can you clarify what you mean by injecting moduleExists?
Are you saying to do it similarly to how the entity manager is getting passed into the constructor?
So it would end up being something like this:
protected $entityManager;
protected $moduleExistsMethod;
public function __construct(EntityManager $entity_manager, $moduleExists) {
$this->entityManager = $entity_manager;
$this->moduleExistsMethod = $moduleExists;
}
... and later in nodeShow ...
// Update the history table, stating that this user viewed this node.
if (call_user_func($this->moduleExistsMethod, 'history')) {
history_write($node->nid);
}
I'm trying to use http://symfony.com/doc/current/components/dependency_injection/introduct... as a guide...
Is there a better intro?
Thanks!
Comment #17
rbayliss CreditAttribution: rbayliss commentedI think that's pretty much it, except that you'd be injecting the whole module handler. So :
Comment #18
monan CreditAttribution: monan commentedGreat, thanks rbayliss...
Ok, let's try again. Incorporated all suggested changes so far.
Fingers crossed...
Thanks everyone!
Comment #20
rbayliss CreditAttribution: rbayliss commentedAlmost there.
This is where the moduleHandler should get pulled out of the container and injected into the controller. So the second argument should be $container->get('module_handler')
Comment #21
monan CreditAttribution: monan commentedDoh... My bad...
Let's try that one last time...
Thanks!
Comment #23
dawehnerLet's inject the database connection here.
The module handler should be documented as ModuleHandlerInterface
What about type hinting for NodeInterface instead?
format_date is a service now, so it can be injected.
Comment #24
xtfer CreditAttribution: xtfer commentedUnless something has changed in the last week, NodeInterface doesn't actually work for the Entity yet, since the node is still using the NG shim.
Comment #25
tim.plunkettYep, the NodeBCDecorator went in so we can use NodeInterface now.
Comment #27
tim.plunkett#25: node-1987778-25.patch queued for re-testing.
Comment #29
dawehnerAccording to google this is a conroller:
but I guess we want to have something like that:
Comment #30
jibranPostpone on this #2026081: Clean up ForumBreadcrumbBuilder::build().
Comment #31
tim.plunkettLOL @dawehner, best review ever.
Comment #32
jibranSorry for hijacking the issue. Let's see people like the idea in #2026081-1: Clean up ForumBreadcrumbBuilder::build().
Comment #33
dawehnerI don't really like it, that it is now hardcoded to a route. Just imagine what happens if you override the path with a panel (or however it will be called in drupal 8). The route name will most probably change. maybe it is better to use something like $route->attributes->get('system_path')
Comment #35
dawehnerI think we should name the route different as this is about the revision somehow not simple /node/%node
Comment #36
vijaycs85dawehner++
Comment #37
dawehnerOn the long run this should be certainly replaced by a entity count query, so what about a follow up?
Comment #38
jibranLet's change the function name as well.
Comment #39
vijaycs85Not sure we need to change the method name as it is just render node.
Comment #40
tim.plunkettI think it would be best if it were renamed nodeRevisionView() to match.
Comment #41
vijaycs85ok, here is update...
Comment #42
jibranIt is RTBC if we'll fix #33 in #1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service
Comment #43
jibran#1951268: Convert /forum and /forum/% to new router, remove forum_forum_load(), forum_get_topics(), create Forum service is also close let's move this and fix breadcrumb in #2026081: Clean up ForumBreadcrumbBuilder::build().
Comment #44
dawehnerThis documentation is wrong.
This name is bad, as we are dealing with just the revision check.
let's inject the entity manager in order to load the node.
Comment #45
jessebeach CreditAttribution: jessebeach commentedRerolled because of a conflict with node.services.yml.
Comment #46
vijaycs85Comment #47
scor CreditAttribution: scor commentedrerolling after #2039199 was committed.
Comment #48
jessebeach CreditAttribution: jessebeach commentedI address @dawehner three comments in #41 and Crell in chat.
NodeAccessCheck
toNodeRevisionAccessCheck
.node_load()
is removed and replaced with an injected$entityManager
, from which I grab the node storage controller and use to load the node from its ID.$GLOBALS['user']
is replaced with$request->attributes->get('account')
.The crux of the replacement node load code is below. I'm not 100% sure this is best practice. It's the best I could surmise from existing examples.
to this
The
$entityManager
is passed into theNodeRevisionAccessCheck
as a service. I must admit, the logic here has me a bit confused. The comment could be improved by someone who understands what is happening a little better than I.Also, we ran into an issue with
$request->attributes->get('account')
. It doesn't consistently return an object. This issue is being addressed in #2040065: Remove _account from request and use the current user service instead.. Until then I left this @todo in the code.My thinking is that the access check should throw an error for now. Ideally the account object will always be included in the request, so when it is not, we want an error to alert us to this, rather than simply returning false and moving on. If my assumption is wrong; if we can return false when the account object is null without hiding a deeper error, then we can remove the throw and just return false.
Comment #49
tim.plunkettI wrote basically the same thing in #1946434: Convert all of confirm_form() in node.admin.inc and node.pages.inc to the new form interface :(
Comment #50
jessebeach CreditAttribution: jessebeach commentedtim.plunkett, we should just focus on #1946434: Convert all of confirm_form() in node.admin.inc and node.pages.inc to the new form interface then. It seems to be a superset of functionality proposed in this issue here.
Comment #52
jessebeach CreditAttribution: jessebeach commented#1946434: Convert all of confirm_form() in node.admin.inc and node.pages.inc to the new form interface was committed. I'm rerolling this issue to remove the duplicated code.
Comment #53
jessebeach CreditAttribution: jessebeach commentedI'm trying to pull apart the dependencies in these two very similar, but differently structured Classes: NodeAccessController and NodeRevisionAccessCheck.
Comment #54
jessebeach CreditAttribution: jessebeach commentedI've removed the addition of
NodeRevisionAccessCheck
from the patch. It was added in #1946434: Convert all of confirm_form() in node.admin.inc and node.pages.inc to the new form interface.If you read comment #53, you'll see a very subtle inconsistency:
The signatures of these methods have an inverted order for $langcode and AccountInterface. I've updated NodeRevisionAccessCheck::checkAccess to match the code found in other parts of core.
NodeRevisionAccessCheck::checkAccess(NodeInterface, $op, $langcode, AccountInterface)
This required changing a few lines in tests:
I replaced instances of
_access_node_revision
with_node_revision_access
in order to remove inconsistencies.I did not remove the reference to
$GLOBALS['user']
. Getting the account from the request just flat out doesn't work yet, so I left a comment.We are now at least passing the account from the request into the checkAccess function, so once #2040065 is committed we can attempt to remove the if statement indicated by the comment.
And very oddly I had to make the following change:
to remove the $profile variable from the
NodeRevisionsAllTestCase
because if the profile is declared, then NodeTestBase won't set up the default Page and Article content types, so the entire test fails to set up properly. I'm not sure how this is working in HEAD right now :/ But it was blowing up locally for me. Removing that line causes the test suite to run and everything passes.Comment #55
tim.plunkett#54: convert-node-show-1987778-54.patch queued for re-testing.
Comment #57
dokumori CreditAttribution: dokumori commentedRerolling the patch
Comment #59
tim.plunkett#57: 1987778-node-show-controller-57.patch queued for re-testing.
Comment #60
tim.plunkettI don't understand this change.
_access_aggregator_categories
_access_edit_entity_field
_access_edit_entity
_access_menu_delete_menu
_access_overlay_dismiss_message
_access_rest_csrf
_access_shortcut_link_delete
_access_system_cron
_access_taxonomy_term_create
_access_user_register
etc...
Comment #61
BerdirIt should also use appliesTo() now, but without the rename, we no longer touch that method, so probably a separate issue.
Comment #62
Crell CreditAttribution: Crell commentedI've decided it makes sense to merge this issue with #1987768: Convert node_page_view() to a new style controller. The attached patch contains code from both issues, plus some updates.
Committers: Please be sure to also credit berliner, pguillard, and vijaycs85 from the other issue.
It may still fail tests, but let's see.
Also bumping to major as this blocks #2019123: Use the same canonical URI paths as for HTML routes.
Interdiff is against #57, and is mostly the result of merging in the other issue.
Comment #63
Crell CreditAttribution: Crell commentedRetitling.
Comment #65
Crell CreditAttribution: Crell commentedThis should fix all of those notices.
I don't know about some of the fatals. Unfortunately my local test environment seems to be acting up and giving me different errors than what's on testbot. Possibly due to me running PHP 5.4. If someone wants to take a crack at it I'd much appreciate it. :-)
Comment #66
tim.plunkettHow is this related?
Out of scope. Also backwards. I left a big long review on this either here or on the other issue you merged in.
Needs docblock. Also, this has been either $this->translationManager in 80% of core, or $this->translator in the other 20%, but not $this->translation
Isn't this converting node/%?
Isn't that a thing for under requirements?
Comment #68
Crell CreditAttribution: Crell commentedI'm not sure, frankly. It was in the other dupe issue so when I merged them I kept it in. I'm going to assume it's necessary or at least OK to include unless someone pushes back on it or we find it's responsible for some errors.
Attached patch fixes the other issues Tim mentioned. I tried a few of the tests that testbot didn't like, but couldn't replicate. Testing again. I'm really getting sick of testbot and my computer disagreeing.
Comment #70
Crell CreditAttribution: Crell commentedOK, that makes no sense. Someone with 5.3 please lend me a hand here. :-/
Comment #71
tim.plunkettPretty sure this has nothing to do with 5.3, unless array keys work different in the future ;)
Comment #72
Crell CreditAttribution: Crell commentedWTF? Then why did those tests pass for me? *sigh* Well, someone needs to review/RTBC this now. :-) Thanks Tim.Scratch that. I really need to pay more attention. That's not even the right patch for this issue. *sigh*
Comment #73
tim.plunkettWrong patch, correct interdiff. My apologies.
Comment #75
tim.plunkettThis either needs
_access_mode: 'ALL'
or should wait for #2063401: Replace the default _access_checks(access mode) with ALL instead of ANYComment #76
Crell CreditAttribution: Crell commentedWhy? The regex checks are not handled by the access control system. They're part of the UrlMatcher and would result in a 404, not 403.
Comment #77
tim.plunkettOkay, well it seemed to be an easy explanation by those fails. I leave it to someone else to debug then.
Comment #78
tim.plunkettHere's some fixes.
Comment #80
jibranIt needs reroll.
Comment #81
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 #82
jibranPlease also clean
\Drupal\comment\Form\DeleteForm
in next reroll.Comment #83
kim.pepperThis is pretty much a rewrite after #2089635: Convert non-test non-form page callbacks to routes and controllers went in.
It only focusses on node_show() and node_page_view() and doesn't touch breadcrumbs or anything else. I moved the methods in \Drupal\node\Controller\NodeView back into \Drupal\node\Controller\NodeController as they were pretty closely tied together.
Comment #84
kim.pepperDoesn't need a reroll anymore
Comment #85
dawehnerIs there a reason why these two functions are not removed in this issue?
Wow I would have not thought that this went throw an actual review given that inline JS seems really wrong.
Comment #86
damiankloip CreditAttribution: damiankloip commentedGood points, I think maybe that inline js should be tackled in another issue. It definitely seems crappy to me :)
Looking and the buildPage method; it seems like node should not be caring about this, and history module should be altering the node build array instead.
Also, removed those functions.
Comment #88
Crell CreditAttribution: Crell commentedhook_node_view_alter() can be invoked from places other than hitting /node/{node}; I think it's called for any teaser or other view mode, too. This is going to end up over-reporting a user's hits.
Comment #89
tim.plunkettYeah, like any view using teasers of a node.
It could at least check the view mode (for 'full'), or check the route name?
Comment #90
damiankloip CreditAttribution: damiankloip commentedHa, you guys are totally right. I didn't think about that, but then I didn't really spend a lot of time on that patch :p
Comment #91
dawehnerI prefer to use the display mode only as you could see the full display in various places which then should count it.
It would be great to add a @todo to remove this uglyness.
Comment #92
dawehnerUps,
Comment #94
kim.pepperYou removed build page, but we're still calling it here.
Comment #95
damiankloip CreditAttribution: damiankloip commentedRerolled and added that todo.
Kim, you're totally right, that would be why the revision tests are failing I guess :) I added back in the buildPage() method, sorry for removing that...
Comment #96
Wim Leers#85 & #86: The inline JS got through extensive reviews at #1991684: Node history markers (comment & node "new" indicator, "x new comments" links) forces render caching to be per user — please read that issue before jumping to conclusions. That being said, obviously I agree that it's ugly. But at least now node views are render cacheable ;) Until we have something like a
#post_render_cache
callback, this is the only way we can fix it. See #2073535: Node history markers require an HTTP request .Comment #97
damiankloip CreditAttribution: damiankloip commentedThanks for the heads up Wim. I'm not going to read that issue though, I will just take your word for it.. I believe you on these matters :)
Comment #98
Crell CreditAttribution: Crell commentedI guess we should remove the @todo then.
Looking through, I didn't see anything else to complain about. Yay for code finally working! :-)
Comment #99
damiankloip CreditAttribution: damiankloip commentedTotally.
Comment #100
dawehnerGreat, let's try to continue on the other few ones.
Comment #101
catchCouldn't this use #attached?
Otherwise seems OK.
Comment #102
catchComment #103
tim.plunkettNo? We could change drupal_process_attached to support more than just 'library', 'css', or 'js', but I think that's out of scope.
Comment #104
catchIt already does:
Comment #105
dawehnerLet's do that and test that it works.
Comment #106
kim.pepperLooks good. Back to RTBC.
Comment #107
catchThanks! Committed/pushed to 8.x.
Comment #108
andypostFollow-up to fix @todo #2111419-4: Remove CommentManager::getParentEntityUri() in favor of Comment::permalink()
Comment #109.0
(not verified) CreditAttribution: commentedAdd note to committers for commit credits
Comment #110
andypostTodos are removed in #2245001: Remove unneeded CommentManagerInterface::getParentEntityUri()