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

Files: 
CommentFileSizeAuthor
#30 bundle-request-argument-2068287-30.patch6.87 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,371 pass(es).
[ View ]
#25 bundle-request-argument-2068287-25.patch6.87 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,406 pass(es).
[ View ]
#25 bundle-request-argument-2068287-25-interdiff.txt783 bytesBerdir
#23 bundle-request-argument-2068287-23.patch6.87 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,501 pass(es).
[ View ]
#23 bundle-request-argument-2068287-23-interdiff.txt1.03 KBBerdir
#14 bundle-request-argument-2068287-14-interdiff.txt4.4 KBBerdir
#14 bundle-request-argument-2068287-14.patch6.9 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 58,079 pass(es).
[ View ]
#14 bundle-request-argument-2068287-14-tests-only.patch3.46 KBBerdir
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
#7 bundle-request-argument-2068287-7.patch3.1 KBBerdir
PASSED: [[SimpleTest]]: [MySQL] 57,984 pass(es).
[ View ]
#1 2068287-1.patch2.16 KBandypost
PASSED: [[SimpleTest]]: [MySQL] 58,110 pass(es).
[ View ]

Comments

andypost’s picture

Status:Active» Needs review
Issue tags:+API clean-up
StatusFileSize
new2.16 KB
PASSED: [[SimpleTest]]: [MySQL] 58,110 pass(es).
[ View ]

Suppose that enough

Status:Needs review» Needs work
Issue tags:-API clean-up, -Entity Access

The last submitted patch, 2068287-1.patch, failed testing.

andypost’s picture

Status:Needs work» Needs review
Issue tags:+API clean-up, +Entity Access

#1: 2068287-1.patch queued for re-testing.

Berdir’s picture

Status:Needs review» Needs work

This 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.

andypost’s picture

@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

@@ -1,35 +0,0 @@
-  public function access(Route $route, Request $request) {
...
-      return $this->entityManager->getAccessController($entity_type)->createAccess($vocabulary->id());
...
-    return parent::access($route, $request);

There's no check for access to bundle so the class is useless

Berdir’s picture

Just 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.

Berdir’s picture

Title:Fix TermAccessController::checkCreateAccess() to properly check $context» Support bundle names provided in the request arguments in EntityCreateAccessCheck
Component:taxonomy.module» entity system
Category:bug» task
Status:Needs work» Needs review
Issue tags:+Needs tests
StatusFileSize
new3.1 KB
PASSED: [[SimpleTest]]: [MySQL] 57,984 pass(es).
[ View ]

This 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.

andypost’s picture

Nice idea!

@@ -52,6 +52,15 @@ public function appliesTo() {
   public function access(Route $route, Request $request) {
     list($entity_type, $bundle) = explode(':', $route->getRequirement($this->requirementsKey) . ':');
...
+    if ($bundle && strpos($bundle, '{') !== FALSE) {
+      foreach ($request->get('_raw_variables')->all() as $name => $value) {
+        $bundle = str_replace('{' . $name . '}', $value, $bundle);

Also that code should make sure that bundle is needed for this kind of entity

Status:Needs review» Needs work

The last submitted patch, bundle-request-argument-2068287-7.patch, failed testing.

andypost’s picture

Also the bundle could be received via HtmlEntityFormController::getFormObject() by providing a key for default section of the route

<?php
//- $entity = $manager->getStorageController($entity_type)->create(array());
//++
if ($bundle_key = entity_has_bundle_info_key()){
 
$bundle_name = entity_get_bundle_from_attributes()->id();
 
$entity = $manager->getStorageController($entity_type)->create(array($bundle_key => $bundle_name));
}
?>

EDIT and EntityRouteEnhancer::enhance() could get defaults for bundle also... not sure

It seems that only fields knows their bundle

Berdir’s picture

Changing 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.

Berdir’s picture

Status:Needs work» Needs review
dawehner’s picture

+1 in general, beside yeah this needs for sure some tests.

Berdir’s picture

Issue tags:-Needs tests+Entity Field API
StatusFileSize
new3.46 KB
FAILED: [[SimpleTest]]: [MySQL] Failed to run tests: PHP Fatal error encountered during run_tests.sh. See review log for details..
[ View ]
new6.9 KB
PASSED: [[SimpleTest]]: [MySQL] 58,079 pass(es).
[ View ]
new4.4 KB

Here are tests :)

Status:Needs review» Needs work

The last submitted patch, bundle-request-argument-2068287-14-tests-only.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

Grml.

dawehner’s picture

Just 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

+++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php
@@ -52,7 +52,20 @@ public function appliesTo() {
+      foreach ($request->get('_raw_variables')->all() as $name => $value) {
+        $bundle = str_replace('{' . $name . '}', $value, $bundle);
+      }

These lines are a bit odd. Wouldn't it be possible to extract the placeholder and directly get it from the _raw_variables?

Berdir’s picture

Yes 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()...

andypost’s picture

Status:Needs review» Needs work

Suppose we could use additional checking for 'bundle_of' according https://drupal.org/node/2073793

andypost’s picture

Status:Needs work» Needs review

Also having {} around seems useless, because we use dot mostly allover

Berdir’s picture

bundle_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.

Berdir’s picture

Double post.

Berdir’s picture

StatusFileSize
new1.03 KB
new6.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,501 pass(es).
[ View ]

Re-roll, as that other issue went in. Tried to improve the comments a bit.

tim.plunkett’s picture

Issue tags:+blocker
+++ b/core/tests/Drupal/Tests/Core/Entity/EntityCreateAccessCheckTest.php
@@ -65,6 +66,12 @@ public function providerTestAccess() {
+      array('', 'entity_test:{bundle_argument}',TRUE, AccessCheckInterface::DENY),

missing space between ,TRUE

Otherwise, great! #1987762: Convert node_add_page() to a new style controller could use this.

Berdir’s picture

StatusFileSize
new783 bytes
new6.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,406 pass(es).
[ View ]

Fixed that whitespace ;)

tim.plunkett’s picture

Status:Needs review» Reviewed & tested by the community

Thanks!

andypost’s picture

Status:Reviewed & tested by the community» Needs review
+++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.php
@@ -52,6 +52,19 @@ public function appliesTo() {
   public function access(Route $route, Request $request) {
     list($entity_type, $bundle) = explode(':', $route->getRequirement($this->requirementsKey) . ':');
...
+    if ($bundle && strpos($bundle, '{') !== FALSE) {

$bundle never get a value so the first condition is always FALSE

andypost’s picture

Status:Needs review» Reviewed & tested by the community

Talked @bedir at IRC - no reason to hold patch on menu_link messy implementation

alexpott’s picture

Status:Reviewed & tested by the community» Needs work
Issue tags:+Needs reroll

Patch no longer applies.

Berdir’s picture

Status:Needs work» Needs review
StatusFileSize
new6.87 KB
PASSED: [[SimpleTest]]: [MySQL] 58,371 pass(es).
[ View ]

Yes, taxonomy.services.yml, I still want to delete you.

Berdir’s picture

Status:Needs review» Reviewed & tested by the community

No functional changes, just re-deleted a file that went from plugin.manager.entity to entity.manager.

alexpott’s picture

Priority:Normal» Critical
Status:Reviewed & tested by the community» Active
Issue tags:-Needs reroll+Needs change record

Committed 1903e50 and pushed to 8.x. Thanks!

I think we need to update https://drupal.org/node/1862420

catch’s picture

Title:Support bundle names provided in the request arguments in EntityCreateAccessCheck» Change notice: Support bundle names provided in the request arguments in EntityCreateAccessCheck
Priority:Critical» Major
Berdir’s picture

Title:Change notice: Support bundle names provided in the request arguments in EntityCreateAccessCheck» Support bundle names provided in the request arguments in EntityCreateAccessCheck
Priority:Major» Normal
Status:Active» Fixed
Issue tags:-Needs change record

Trying 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.

Automatically closed -- issue fixed for 2 weeks with no activity.