Support from Acquia helps fund testing for Drupal Acquia logo

Comments

dawehner’s picture

Status: Active » Needs review
Issue tags: +PHPUnit
FileSize
7.64 KB
tim.plunkett’s picture

I was hoping to use this for #1987860: Convert taxonomy_term_add() to a new style controller, but it seems like the bundle value must be hardcoded?
I can't see when that's actually useful.

There are only two usages of entity_page_create_access() in core,

Either it doesn't pass a bundle:
entity_page_create_access('taxonomy_vocabulary') (via access callback/access arguments for admin/structure/taxonomy/add)

Or pass a dynamic value:

function taxonomy_term_create_access(Vocabulary $vocabulary) {
  return entity_page_create_access('taxonomy_term', $vocabulary->id());
}

(for admin/structure/taxonomy/manage/%taxonomy_vocabulary/add)

Obviously we should refactor other parts of core to use the more generic approach when possible, but I'm not sure we'll ever get this working for bundles.

tim.plunkett’s picture

What about adding a protected method to this for subclasses to easily define what goes in their values?

Then you can register your own subclass of this and override like so:

protected function prepareEntityValues(array $values, array $definition, Request $request) {
  $values['vid'] = $request->attributes->get('taxonomy_vocabulary')->id();
  return $values;
}
tim.plunkett’s picture

If we don't choose to do what I suggest in #3, let's just roll what #1 does directly into EntityCheckAccess when $operation == 'create'.

dawehner’s picture

FileSize
1.98 KB
8.26 KB

It seems to be that moving the functionality into the method makes actually quite amount of sense.

Status: Needs review » Needs work

The last submitted patch, access-2028585-5.patch, failed testing.

tim.plunkett’s picture

I considered doing that. That's probably okay, you can just not call parent:: if you don't care.
That's a random fail, going to retest.

tim.plunkett’s picture

Status: Needs work » Needs review

#5: access-2028585-5.patch queued for re-testing.

pfrenssen’s picture

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

looks good..we might not have any usages of it, but we have tests, and they are pretty solid. rtbc?

tim.plunkett’s picture

RTBC +1, we can immediately use this in both taxonomy conversions.

ParisLiakos’s picture

FileSize
521 bytes
8.32 KB

small missing docblock!

The last submitted patch, access-2028585-12.patch, failed testing.

Status: Reviewed & tested by the community » Needs work
Issue tags: -PHPUnit

The last submitted patch, access-2028585-12.patch, failed testing.

ParisLiakos’s picture

Status: Needs work » Needs review
Issue tags: +PHPUnit

#12: access-2028585-12.patch queued for re-testing.

twistor’s picture

+++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.phpundefined
@@ -0,0 +1,87 @@
+  public function access(Route $route, Request $request) {
+    $split = explode(':', $route->getRequirement('_entity_create_access'));
+    if (count($split) == 2) {
+      list($entity_type, $bundle) = $split;
+    }
+    else {
+      $entity_type = array_pop($split);
+      $bundle = '';

This could be simplified quite a bit.
list($entity_type, $bundle) = explode(':', $route->getRequirement('_entity_create_access') . ':')

+++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.phpundefined
@@ -0,0 +1,87 @@
+    $values = $this->prepareEntityValues($definition, $request, $bundle);
+    // Pass in the entity bundle if given and required.
+    $entity = $this->entityManager->getStorageController($entity_type)

This comment is out of place.

+++ b/core/lib/Drupal/Core/Entity/EntityCreateAccessCheck.phpundefined
@@ -0,0 +1,87 @@
+  /**
+   * Prepare the values passed into the storage controller.
+   *
+   * @param array $definition
+   *   The entity type definition.
+   * @param \Symfony\Component\HttpFoundation\Request $request
+   *   The request object.
+   * @param string $bundle
+   *   (optional) The bundle to check access for. Defaults to an empty string.
+   *
+   * @return array
+   *   The modified values to be used to create the entity.
+   */
+  protected function prepareEntityValues(array $definition, Request $request, $bundle = '') {

Why is the request being passed in here?

$bundle should default to NULL.

The return docs are incorrect.

Leaving as NR to see test results.

tim.plunkett’s picture

The Request is needed to do anything useful in a subclass. See #3.

The rest is valid.

ParisLiakos’s picture

FileSize
1.87 KB
8.1 KB

this should take care of points above

dawehner’s picture

+++ b/core/tests/Drupal/Tests/Core/Entity/EntityCreateAccessCheckTest.phpundefined
@@ -0,0 +1,136 @@
+class EntityCreateAccessCheckTest extends UnitTestCase {

Missing docs

ParisLiakos’s picture

FileSize
632 bytes
8.24 KB

docs added

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thanks

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0af43e4 and pushed to 8.x. Thanks!

tim.plunkett’s picture

Status: Fixed » Needs review
FileSize
1.33 KB

I feel stupid. This isn't good enough. We need this to do anything sufficiently complex.

See #1987860-10: Convert taxonomy_term_add() to a new style controller for an example.

ParisLiakos’s picture

Status: Needs review » Reviewed & tested by the community

i had to check the other issue to figure out why this is needed, as well:/

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 0453593 and pushed to 8.x. Thanks!

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