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.
Updated: Comment #0
Problem/Motivation
TaxonomyTermCreateAccess
is only works for route but should work for REST and other entity related operations
Proposed resolution
Move logic from TaxonomyTermCreateAccess
to TermAccessController::checkCreateAccess()
API changes
TaxonomyTermCreateAccess
should be removed as useless
Comment | File | Size | Author |
---|---|---|---|
#30 | bundle-request-argument-2068287-30.patch | 6.87 KB | Berdir |
#25 | bundle-request-argument-2068287-25.patch | 6.87 KB | Berdir |
#25 | bundle-request-argument-2068287-25-interdiff.txt | 783 bytes | Berdir |
#23 | bundle-request-argument-2068287-23.patch | 6.87 KB | Berdir |
#23 | bundle-request-argument-2068287-23-interdiff.txt | 1.03 KB | Berdir |
Comments
Comment #1
andypostSuppose that enough
Comment #3
andypost#1: 2068287-1.patch queued for re-testing.
Comment #4
BerdirThis only works because Core currently doesn't implement bundle-specific permissions. But this is exactly what the access class does, making sure that the bundle is passed through to checkCreateAccess().
I don't see how this is in the way for REST?
What I'd be happy todo is add something like _entity_bundle_argument: taxonomy_vocabulary and support this in the default implementation. Nodes and custom blocks will need this too and the same would be useful to have for the add form itself, where we need to create the entity with the right bundle.
Comment #5
andypost@Berdir bundle is passed to checkCreateAccess() same way but currently via wrapper class
Agree that there's no check for bundle.
Probably I need file another issue to address what you point and limit scope of the issue just to remove useless access checker
There's no check for access to bundle so the class is useless
Comment #6
BerdirJust because core doesn't need it doesn't mean it's useless :)
See for example https://drupal.org/project/taxonomy_access_fix, people also tried to get that into core. createAccess() also invokes a create_access hook that modules can implement.
All create access checks for entity types that have bundles should IMHO specific the bundle, when possible.
Comment #7
BerdirThis is quite different from the original patch, but I think this is the only way to get rid of that class without losing functionality.
Worked fine in a manual test, needs unit tests.
Comment #8
andypostNice idea!
Also that code should make sure that bundle is needed for this kind of entity
Comment #10
andypostAlso the bundle could be received via
HtmlEntityFormController::getFormObject()
by providing a key for default section of the routeEDIT and
EntityRouteEnhancer::enhance()
could get defaults for bundle also... not sureIt seems that only fields knows their bundle
Comment #11
BerdirChanging other classes should IMHO happen in a separate issue. But yes, agreed that they should support the same syntax somehow. Already pinged @larowlan about this issue.
Comment #12
Berdir#7: bundle-request-argument-2068287-7.patch queued for re-testing.
Comment #13
dawehner+1 in general, beside yeah this needs for sure some tests.
Comment #14
BerdirHere are tests :)
Comment #16
BerdirGrml.
Comment #17
dawehnerJust referencing another issue which does the same change: #2063405: Update all access checkers to use static::ALLOW/static::DENY/static::KILL
I am wondering whth
These lines are a bit odd. Wouldn't it be possible to extract the placeholder and directly get it from the _raw_variables?
Comment #18
BerdirYes I thought I include that here as well as I touched that code anyway. It's weird to explicitly test for the constant but not return it.
Well yes, I guess it would be possible, but that was the easiest version I could think of. Extracting and then replacing will still need the same str_replace()...
Comment #19
andypostSuppose we could use additional checking for 'bundle_of' according https://drupal.org/node/2073793
Comment #20
andypostAlso having {} around seems useless, because we use dot mostly allover
Comment #21
Berdirbundle_of doesn't help here, it's optional, there's no point in checking that.
The {} are important, they different between a bundle named taxonomy_vocabulary and a request argument with that name.
Comment #22
BerdirDouble post.
Comment #23
BerdirRe-roll, as that other issue went in. Tried to improve the comments a bit.
Comment #24
tim.plunkettmissing space between ,TRUE
Otherwise, great! #1987762: Convert node_add_page() to a new style controller could use this.
Comment #25
BerdirFixed that whitespace ;)
Comment #26
tim.plunkettThanks!
Comment #27
andypost$bundle never get a value so the first condition is always FALSE
Comment #28
andypostTalked @bedir at IRC - no reason to hold patch on menu_link messy implementation
Comment #29
alexpottPatch no longer applies.
Comment #30
BerdirYes, taxonomy.services.yml, I still want to delete you.
Comment #31
BerdirNo functional changes, just re-deleted a file that went from plugin.manager.entity to entity.manager.
Comment #32
alexpottCommitted 1903e50 and pushed to 8.x. Thanks!
I think we need to update https://drupal.org/node/1862420
Comment #33
catchComment #34
BerdirTrying to keep up with the change notices I have to write.
Turns out _entity_create_access never got a change notice, so I extend the existing one about _entity_access about it and this specific feature here: https://drupal.org/node/1982078.