Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use fully loaded node entities in routes starting with /node/{node} #12

Closed
wants to merge 3 commits into from

Conversation

qadan
Copy link

@qadan qadan commented May 12, 2020

Passing in the node ID as a string in routes is causing issues with modules that expect routes that are part of /node/{node} to have that {node} be a loaded node entity.

This can be demonstrated (and was initially observed) by enabling the Permissions by Term module, which grabs the node out of the route to determine access. Since the EmbargoesNodeEmbargoesController gets a string instead of an entity object, the node entity isn't available from the route parameters, and the call to $node->id() fails.

I'm not sure I can dig up any standards describing this as being a hard requirement, but since /node/{node} comes from the node module itself, it would make sense to align any new sub-routes with how the node module does things. It's also worth mentioning that the {node} that gets passed into the embargoes.node.embargo route is already a Drupal\node\Entity\Node object despite not having been specified, which does reinforce that this behaviour is expected.

This PR is to update the embargoes.node.embargoes route to use a node entity, to accept that node entity on the other end, and to update the EmbargoesNodeEmbargoesForm to document what's being passed in.

@qadan qadan changed the title nodes as entities Use fully loaded node entities in routes starting with /node/{node} May 14, 2020
@qadan
Copy link
Author

qadan commented Aug 10, 2020

Superseded by #15

@qadan qadan closed this Aug 10, 2020
qadan pushed a commit to qadan/embargoes that referenced this pull request Aug 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants