Support from Acquia helps fund testing for Drupal Acquia logo

Comments

tim.plunkett’s picture

pfrenssen’s picture

Assigned: Unassigned » pfrenssen

I'll try my hand at this.

pfrenssen’s picture

Status: Active » Needs review
FileSize
3.37 KB

Here's a first version. I still have to figure out how to port the access callback. The example does not cover this. Currently the user gets an access denied when visiting admin/structure/taxonomy/add.

pfrenssen’s picture

Updated version of the patch. Implemented the access check but it is not triggered. User still gets access denied.

Status: Needs review » Needs work

The last submitted patch, 1987866-4-taxonomy-vocabulary_add_controller.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
4.31 KB

Restored the hook_menu() entry and added the route_name to it. Fixed the route_name in the routing.yml file. Still no luck.

Status: Needs review » Needs work

The last submitted patch, 1987866-6-taxonomy-vocabulary_add_controller.patch, failed testing.

pfrenssen’s picture

Status: Needs work » Needs review
FileSize
3.28 KB

Discovered VocabularyAccessController. I'm learning lots of interesting new things here, will get there in the end :)

Status: Needs review » Needs work

The last submitted patch, 1987866-8-taxonomy-vocabulary_add_controller.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Re-roll after #1891690: Use EntityListController for vocabularies landed and updating callback with _entity_form. Changing ACTION_CALLBACK to hook as mentioned in [#2007444] . For access we need to use entity_page_create_access(), however it is not converted to new routing access class. For now setting access as 'administer taxonomy'.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion, -MENU_LOCAL_ACTION

The last submitted patch, 1987866-taxonomy-vocabulary_add_controller-10.patch, failed testing.

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +WSCCI-conversion, +MENU_LOCAL_ACTION
andypost’s picture

Looks ready

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -240,13 +240,9 @@ function taxonomy_menu() {
   $items['admin/structure/taxonomy/add'] = array(
     'title' => 'Add vocabulary',
...
+    'route_name' => 'taxonomy_vocabulary_add',
+    'type' => MENU_SIBLING_LOCAL_TASK,

is this still needed?

jibran’s picture

andypost’s picture

Status: Needs review » Reviewed & tested by the community

so rtbc

tim.plunkett’s picture

Status: Reviewed & tested by the community » Needs review

You don't need that for the action link itself, but to ensure the local tabs show up on the admin/structure/taxonomy/add itself. They are separate things.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

Crosspost

alexpott’s picture

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

Needs a reroll

curl https://drupal.org/files/1987866-taxonomy-vocabulary_add_controller-10_0.patch | git apply
  % Total    % Received % Xferd  Average Speed   Time    Time     Time  Current
                                 Dload  Upload   Total   Spent    Left  Speed
100  1741  100  1741    0     0   1359      0  0:00:01  0:00:01 --:--:--  1621
error: patch failed: core/modules/taxonomy/taxonomy.routing.yml:5
error: core/modules/taxonomy/taxonomy.routing.yml: patch does not apply
tim.plunkett’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
2.45 KB

Rerolled.

Status: Needs review » Needs work
Issue tags: -WSCCI-conversion, -MENU_LOCAL_ACTION

The last submitted patch, taxonomy-1987866-19.patch, failed testing.

pwieck’s picture

Status: Needs work » Needs review

#19: taxonomy-1987866-19.patch queued for re-testing.

Had this same error earlier today on another issue. After re-test it passed.

Status: Needs review » Needs work
Issue tags: +WSCCI-conversion, +MENU_LOCAL_ACTION

The last submitted patch, taxonomy-1987866-19.patch, failed testing.

andypost’s picture

Issue tags: +D8MI
+++ b/core/modules/taxonomy/taxonomy.admin.incundefined
@@ -9,18 +9,6 @@
-  $vocabulary = entity_create('taxonomy_vocabulary', array(
-    // Default the new vocabulary to the site's default language. This is the
-    // most likely default value until we have better flexible settings.
-    'langcode' => language_default()->langcode,

+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -4,3 +4,10 @@ taxonomy_vocabulary_list:
+    _entity_form: 'taxonomy_vocabulary'

We drop this code, is this still needed?

tim.plunkett’s picture

Status: Needs work » Needs review

That line is covered by Term::$values, which is merged in during EntityNG::__construct().

pfrenssen’s picture

Assigned: pfrenssen » Unassigned
Berdir’s picture

#19: taxonomy-1987866-19.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8MI, +WSCCI-conversion, +MENU_LOCAL_ACTION

The last submitted patch, taxonomy-1987866-19.patch, failed testing.

Berdir’s picture

Status: Needs work » Needs review
+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -241,11 +241,8 @@ function taxonomy_menu() {
-    'access callback' => 'entity_page_create_access',

+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -4,3 +4,10 @@ taxonomy_vocabulary_list:
+    _permission: 'administer taxonomy'

I guess it's unlikely that someone wants to alter this through the access controller/hook but still a bit unfortunate to introduce that inconsistency shortly after we managed to make it consistent ;)

We don't have a way to replace entity_page_create_access yet, not sure how to do it. As we optionally need to support the bundle too.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -1304,3 +1301,18 @@ function taxonomy_library_info() {
+ * Implements hook_local_actions.

() missing ;)

benjf’s picture

FileSize
2.47 KB

patch re-roll attempt:)

I also added the 2nd part of comment #28 above, the missing ().

andypost’s picture

+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -4,3 +4,10 @@ taxonomy_vocabulary_list:
+    _permission: 'administer taxonomy'

should be _entity_access: taxonomy_vocabulary.add

pwieck’s picture

Assigned: Unassigned » pwieck

Fixing patch #29 to reflex #30

vijaycs85’s picture

@andypost and @pwieck, may be not for add :( we can't use _entity_access for creating entity.

pwieck’s picture

Assigned: pwieck » Unassigned

NOT fixing patch #29 to reflex #30 per #32...I think?

dawehner’s picture

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -241,11 +241,8 @@ function taxonomy_menu() {
-    'access callback' => 'entity_page_create_access',
-    'access arguments' => array('taxonomy_vocabulary'),

Well at least for constant bundles it would be possible to support it, but for a dynamic issue like taxonomy_term_create_access has, there is no way around custom implementation.

Here is an issue for that #2028585: Replace entity_page_create_access by an access controller though it is certainly not matching every use case.

andypost’s picture

dawehner’s picture

Status: Needs review » Needs work
+++ b/core/modules/taxonomy/taxonomy.routing.ymlundefined
@@ -4,3 +4,10 @@ taxonomy_vocabulary_list:
+  requirements:
+    _permission: 'administer taxonomy'

So we should use the new access checker now.

mtift’s picture

FYI: info about the new access checker is in core/modules/taxonomy/lib/Drupal/taxonomy/Access/TaxonomyTermCreateAccess.php

vijaycs85’s picture

Status: Needs work » Needs review
Issue tags: +LONDON_2013_JULY
FileSize
425 bytes
1.65 KB

Updating patch as per #36 and thanks for reference in #37

Status: Needs review » Needs work

The last submitted patch, 19876866-taxonomy-add-controller-38.patch, failed testing.

jair’s picture

Issue tags: +Needs reroll
dawehner’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
3.04 KB

Let's convert it to a local action plugin.

Status: Needs review » Needs work
Issue tags: -D8MI, -WSCCI-conversion, -MENU_LOCAL_ACTION, -LONDON_2013_JULY

The last submitted patch, taxonomy-1987866-41.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review

#41: taxonomy-1987866-41.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +D8MI, +WSCCI-conversion, +MENU_LOCAL_ACTION, +LONDON_2013_JULY

The last submitted patch, taxonomy-1987866-41.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1014 bytes
3.05 KB

Ups.

andypost’s picture

FileSize
3.44 KB
3.21 KB

Suppose better use yml file for local action.
Also access check should use proper implementation

EDIT sorry for xpost
Related #1966436: Default *content* entity languages are not set for entities created with the API

jibran’s picture

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Plugin/Core/Entity/Vocabulary.php
@@ -25,7 +25,8 @@
- *       "default" = "Drupal\taxonomy\VocabularyFormController",
+ *       "add" = "Drupal\taxonomy\VocabularyFormController",
+ *       "edit" = "Drupal\taxonomy\VocabularyFormController",

Let's exclude this and add the interdiff from #46

andypost’s picture

Status: Needs review » Needs work

The last submitted patch, taxonomy-1987866-45.patch, failed testing.

andypost’s picture

Status: Needs work » Needs review
FileSize
2.59 KB
1.28 KB

Let's see how default actions affects the patch

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Checked manually worked fine.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed c534b44 and pushed to 8.x. Thanks!

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