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

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.

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

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

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.

@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

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.

Title:Fix TermAccessController::checkCreateAccess() to properly check $contextSupport 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.

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.

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

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.

Status:Needs work» Needs review

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

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.

Status:Needs work» Needs review

Grml.

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?

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

Status:Needs review» Needs work

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

Status:Needs work» Needs review

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

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.

Double post.

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.

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.

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

Fixed that whitespace ;)

Status:Needs review» Reviewed & tested by the community

Thanks!

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

Status:Needs review» Reviewed & tested by the community

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

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

Patch no longer applies.

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.

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.

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

Title:Support bundle names provided in the request arguments in EntityCreateAccessCheckChange notice: Support bundle names provided in the request arguments in EntityCreateAccessCheck
Priority:Critical» Major

Title:Change notice: Support bundle names provided in the request arguments in EntityCreateAccessCheckSupport 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.