Add a dedicated permission to access the term overview page (without 'administer taxonomy' permission)

This will also deceprate http://drupal.org/project/taxonomy_access_fix right?

edit/delete and related links

and keep edit/delete terms by vocabulary permission as per
#340652: Edit/delete terms permission per vocabulary
and
#1839604: Add taxonomy term permission

the whole motivation behind taxonomy access fix module was to mainly provide Add terms per vocabulary permission.
permit users to Add terms to a given vocabulary whom they are allowed to, as set by the admin, without being able to do that to all vocabularies or administer them.

Edit terms and Delete terms are the complements to Add, respectively for editing and deleting terms - these are already implemented in core and are just noted for reference.

as for Access taxonomy overview page permission, its a secondary goal - important, but not as crucial as Add terms in my opinion

i'm invioning permissions this way:
- Administer vocabularies and terms
- Access taxonomy overview page
- Add terms in (...)
- Edit terms in (...)
- Delete terms (...)

If the patch in #18 is accepted into D8 core will it be added to D7 core as well or will that require a separate issue?

OK, sure. For anyone looking for a D7 solution, check out http://drupal.org/node/1038330#comment-6528288 for a core patch and http://drupal.org/project/taxonomy_access_fix for a module. I opted for the latter and it seems to work fine.

Isn't #1038330: Allow specific vocabulary permissions to work on vocabulary admin pages a duplicate?

@#19: Someone will review the patch sooner or later..when we all think it is ready we will RTBC it and then a core maintainer will commit it.

I willl try to review it , but no promises till weekend

yes, it should imply that =)
good work!

This looks awesome!

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -87,11 +87,22 @@ function taxonomy_help($path, $arg) {
+      'create terms in ' . $vocabulary->vid => array(
+        'title' => t('Add terms in %vocabulary', array('%vocabulary' => $vocabulary->name)),
+      ),
+    );

Hmm why dont we use the vocabulary machine name here?
Eg what node does is:
Create %type content
What dont we do sth similar
Create %vocabulary_machine_name terms.
Unless this is the pattern we already use in taxonomy, so just ignore this

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -369,25 +381,40 @@ function taxonomy_admin_paths() {
-function taxonomy_term_access($op, $term) {
-  if (!$term || !in_array($op, array('edit', 'delete'), TRUE)) {
-    // If there was no term to check against, or the $op was not one of the
+function taxonomy_term_access($op, $obj) {
+  if (!$obj || !in_array($op, array('create', 'edit', 'delete'), TRUE)) {
+    // If there was no object to check against, or the $op was not one of the
     // supported ones, we return access denied.
     return FALSE;
   }
+  return user_access("$op terms in $obj->vid") || user_access('administer taxonomy');
+}

Not a big fan of $obj.
Why dont we keep this argument as $term and initialize it with NULL?
I dont see any special condition for create op anyways

Finally this is gonna need some tests for new permissions i think

ah..i see..sorry i misread your docblock, so nevermind this.
Anyways $obj is not a good name, should be $object..Dunno if we could find a better name for DX.

About tests:
Exactly...and maybe also the opposite, if you could and have time

Edit: about permission names, i found this:
#995156: Use vocabulary machine name for permissions which was closed for this #1552396: Convert vocabularies into configuration

Closed #1034600: Split Taxonomy permissions into 'Vocabulary' and 'Terms' as duplicate. Thanks rootatwc!

heads up..
#1552396: Convert vocabularies into configuration commited

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->admin_user = $this->drupalCreateUser(array('administer taxonomy'));
...
+    $this->vocabulary = $this->createVocabulary();

If you store something on the object you should probably document it.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->assertText(t('Vocabulary name'), 'Vocabulary overview opened successfully.');

You could check assertResponse(200); to check whether the user got access or not.

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+  function testVocabularyPermissionsTaxonomyOverviewUserFail() {

You could do this in the previous test function already, no need for a full new drupal installation to test that :)

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->assertNoText(t('Vocabulary name'), 'Vocabulary overview opened successfully.');

Here you can then check assertResponse(404);

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+  function testVocabularyPermissionsTaxonomyOverviewUserSucceed() {
...
+  function testVocabularyPermissionsTaxonomyTermAdmin() {

similar to above. In general I think it's helpful to keep the documentation you have written

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->drupalGet('taxonomy/term/' . $term->tid . '/edit');

A small possible simplification, you could use $tterm->uri()

+++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
@@ -0,0 +1,203 @@
+    $this->assertText($edit['name'], 'Edit taxonomy term form opened successfully.');

Yeah another place to check access directly.

+++ b/core/modules/taxonomy/taxonomy.moduleundefined
@@ -370,25 +382,48 @@ function taxonomy_admin_paths() {
+function taxonomy_term_access($op, $object) {

This feels wrong, why not have two differnt access functions, one for terms one for vocabularies?

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/TaggedWithTest.phpundefined
@@ -38,6 +38,8 @@ public static function getInfo() {
+    $this->permissions[] = 'administer taxonomy';

What about moving it after the parent setup? This is just convenience.

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/TaggedWithTest.phpundefined
@@ -38,6 +38,8 @@ public static function getInfo() {
+    ¶

whitespace error ...

+++ b/core/modules/views/lib/Drupal/views/Tests/Wizard/WizardTestBase.phpundefined
@@ -13,6 +13,8 @@
+  // Permissions for this user to test with.
+  protected $permissions = array();

Please use proper documentation of this property see drupal.org/node/1354

Also, related: #1862758: [Followup] Implement entity access API for terms and vocabularies
I guess this will get committed sooner than this one, so i am giving the heads up

Well I think having new permissions certainly would help, but yeah this patch has to be rewritten quite a lot.

the other patch is rtbc
once it gets committed i will roll a patch on top of it

the problem as i see it with the current approach is that a user with access taxonomy overview can change the weight of the vocabularies and tags..reorder them and reparent them, even though he might not have permissions to edit them.
Also current tests, fail to catch this, which is a security isssue.

i tried to fix this in top of #1862758: [Followup] Implement entity access API for terms and vocabularies I eventually run out of time and patience. i attach the patch, for anyone that wants to keep it up

Thanks @Johnny vd Laar! Usually when you combine your patch with another, it's best to include your patch by itself (rolled against the other) in a patchname-do-not-test.patch file so that reviewers can tell which code needs to be reviewed.

Nice work on the tests so far. I think it would be better to test each permission separately, to confirm that one isn't bypassing another. Right now we are testing (possibly duplicating tests of) the administer taxonomy permission.

1. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
   @@ -0,0 +1,202 @@
   +  function setUp() {
   +    parent::setUp();
   +  }
   

   This isn't doing anything; it can be removed.

2. +++ b/core/modules/taxonomy/lib/Drupal/taxonomy/Tests/VocabularyPermissionsTest.phpundefined
   @@ -0,0 +1,202 @@
   +    $user = $this->drupalCreateUser(array("administer taxonomy", "edit terms in {$vocabulary->id()}", "delete terms in {$vocabulary->id()}"));
   

   Shouldn't "administer taxonomy" supercede the other two?

With this you mean that we should have a function for "administer taxonomy", one function for "edit terms", etc. And in each function create a user with this permission and without the permission?

It doesn't need to be separate test methods, just separate users in the same test method since we don't need a fresh Drupal installation to log in and out to test separate users. Also, the "administer taxonomy" permission may already have its own test coverage, so you'll want to check for that in the existing tests.

i believe it can go in since its an api addition not an api change, but that would still require for thresholds to be down, which is far from true right now

About the _account thing:
You should use Drupal::currentUser() the attribute in the request had many problems so we switched it to a separate service

As long as I'm talking to myself, nobody will notice me being a n00b ;-)

I noticed! ;)

Patch no longer applies.

Full CRUD permissions would be great.

I will try to reroll the last patch provided in the comments

I'm going to attempt the re-roll

cyborg_572, Are you still working on this?

Ah, I guess not, sorry. My my patch imploded on me (I think I missed a few conflicts before committing), and I haven't had a chance to try again. I'll attach what I had, despite it not working.

Looking at what would be needed to reroll this to current HEAD:

1. AutocompleteTagsWidget no longer exists. This is currently handled by \Drupal\taxonomy\Plugin\EntityReferenceSelection\TermSelection which operates completely differently.
2. AutocompleteWidget no longer exists. This is now handled by \Drupal\Core\Field\Plugin\Field\FieldWidget\EntityReferenceAutocompleteWidget.
3. TermAccessController has become \Drupal\taxonomy\TermAccessControlHandler.
4. VocabularyAccessController no longer exists. Part of its functionality now seems to be covered by \Drupal\taxonomy\TermAccessControlHandler::checkCreateAccess().
5. TaggedWithTest has moved to \Drupal\views\Tests\Wizard\TaggedWithTest.
6. The code in the following classes and files has diverged and needs to be rerolled: EntityReferenceAutoCreateTest, OverviewTerms, VocabularyListBuilder, taxonomy.module, taxonomy.services.yml.

Almost nothing applies any more :-/

This is what a git rebase + manual fixing of the most glaring merge conflicts looks like. This is missing everything from the files that have been removed, but it's a starting point.

Let's see how bad this really is :)

Thanks @pfrenssen for keeping this moving!

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -204,14 +204,23 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+      $empty = $this->t('No terms available. <a href="@link">Add term</a>.', ['@link' => $this->url('entity.taxonomy_term.add_form', ['taxonomy_vocabulary' => $taxonomy_vocabulary->id()])]),

Looks like this should be a semicolon instead of a comma ending the line.

+++ b/core/modules/node/src/Tests/NodeAccessBaseTableTest.php
@@ -99,7 +99,7 @@ function testNodeAccessBasic() {
+      $simple_users[$i] = $this->drupalCreateUser(array('access content', 'create article content', 'create terms in tags'));

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -256,28 +265,36 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+      $operations = array();

@@ -285,10 +302,14 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+        $form['terms'][$key]['operations'] = array(
+          '#type' => 'operations',
+          '#links' => $operations,
+        );

@@ -318,32 +339,40 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+    $form['terms']['#header'] = array($this->t('Name'));

+++ b/core/modules/taxonomy/src/Tests/VocabularyPermissionsTest.php
@@ -15,6 +15,33 @@
+    $authenticated_user = $this->drupalCreateUser(array());
...
+    $proper_user = $this->drupalCreateUser(array('access taxonomy overview'));

@@ -57,8 +84,35 @@ function testVocabularyPermissionsTaxonomyTerm() {
+    $user = $this->drupalCreateUser(array("access taxonomy overview", "create terms in {$vocabulary->id()}"));
...
+    $user = $this->drupalCreateUser(array("access taxonomy overview", "edit terms in {$vocabulary->id()}"));

@@ -82,7 +136,7 @@ function testVocabularyPermissionsTaxonomyTerm() {
+    $user = $this->drupalCreateUser(array("access taxonomy overview", "delete terms in {$vocabulary->id()}"));

+++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
@@ -46,11 +46,14 @@ public function getDefaultOperations(EntityInterface $entity) {
+      $operations['add'] = array(
+        'title' => t('Add terms'),
+        'weight' => 10,
+        'url' => Url::fromRoute('entity.taxonomy_term.add_form', ['taxonomy_vocabulary' => $entity->id()]),
+      );

@@ -83,7 +91,14 @@ public function render() {
+      $build['table']['#empty'] = t('No vocabularies available. <a href="@link">Add vocabulary</a>.', array('@link' => \Drupal::url('entity.taxonomy_vocabulary.add_form')));

+++ b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
@@ -15,6 +15,13 @@
+  protected $permissions = array();

@@ -24,8 +31,10 @@
+    $this->permissions = array_merge($this->permissions, array('administer views', 'administer blocks', 'bypass node access', 'access user profiles', 'view all revisions'));

We should use short array syntax on these.

Fixed the fatal error in OverviewTerms and added the new dynamic permissions to TaxonomyPermissions like is done in current D8 HEAD.

I'm actually also in favor of providing full CRUD permissions in this issue as suggested by @RobLoach in #84. Shall we do that? Would be great for REST integration.

@Manuel Garcia, thanks a lot for the review! This evidently will need a lot of work to get it back in a working state and adhering to our current practices. I just saw your comment now, so I have not yet addressed your remarks.

I agree with #84 as well, it would be very nice!

This should fix a lot of the failing tests.

Fixed failing tests in VocabularyPermissionsTest.

This is the right patch.

Rerolled.

This should fix the failures.

Patch attached.

This patch should apply..

Patch should be green again.

WOW! Great work Jeroen!!

Really excited by this, did a quick "drive by" review in Dreditor during my lunch break.

1. +++ b/core/modules/forum/src/Form/Overview.php
   @@ -67,19 +67,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        if($form['terms'][$key]['#term']->access('delete')) {
   +          unset($form['terms'][$key]['operations']['#links']['delete']);
   

   Coding standards: put a space between the 'if' and the opening parenthesis.

   This also reads strange: if the user has access to delete the term, we remove the delete link?

2. +++ b/core/modules/forum/src/Form/Overview.php
   @@ -67,19 +67,24 @@ public function buildForm(array $form, FormStateInterface $form_state) {
   +        if($form['terms'][$key]['#term']->access('update')) {
   

   Missing space between 'if' and opening parenthesis.

3. +++ b/core/modules/taxonomy/src/Access/TaxonomyOverviewAccessCheck.php
   @@ -0,0 +1,28 @@
   +/**
   + * @file
   + * Contains \Drupal\taxonomy\Access\TaxonomyOverviewAccessCheck
   + */
   

   Missing period at the end of the sentence.

4. +++ b/core/modules/taxonomy/src/Access/TaxonomyOverviewAccessCheck.php
   @@ -0,0 +1,28 @@
   +  /**
   +   * @param AccountInterface $account
   +   * @return AccessResult
   +   */
   

   {@inheritdoc}

5. +++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
   @@ -204,14 +212,23 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
   +      $empty = $this->t('No terms available. <a href="@link">Add term</a>.', ['@link' => $this->url('entity.taxonomy_term.add_form', ['taxonomy_vocabulary' => $taxonomy_vocabulary->id()])]);
   

   For readability I would split out the replacement strings in a separate $args variable.

6. +++ b/core/modules/taxonomy/src/Tests/VocabularyPermissionsTest.php
   @@ -15,6 +15,33 @@
   +  function testVocabularyPermissionsVocabulary() {
   

   This needs more test coverage, the test docblock describes a full CRUD test, but the test actually only tests if a user with the right permission can access the administration page, and that the link is there.

7. +++ b/core/modules/taxonomy/src/Tests/VocabularyPermissionsTest.php
   @@ -15,6 +15,33 @@
   +    // VocabularyTest.php already tests for user with "administer taxonomy" permissions.
   

   Documentation exceeds 80 characters.

8. +++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
   @@ -25,6 +30,51 @@ class VocabularyListBuilder extends DraggableListBuilder {
   +  /**
   +   * Constructs a new VocabularyListBuilder object.
   +   *
   +   * @param \Drupal\Core\Entity\EntityTypeInterface $entity_type
   +   *   The entity type definition.
   +   * @param \Drupal\Core\Entity\EntityStorageInterface $storage
   +   *   The entity storage class.
   +   * @param \Drupal\Core\Entity\EntityManagerInterface $entity_manager
   +   *   The entity manager service.
   +   * @param \Drupal\Core\Session\AccountInterface $current_user
   +   *   The current user.
   +   */
   +  public function __construct(EntityTypeInterface $entity_type, EntityStorageInterface $storage, AccountInterface $current_user, EntityManagerInterface $entity_manager) {
   

   The order of the arguments in the docblock doesn't match the actual order in the function declaration.

This does not meet the beta criteria for 8.0.x. I consulted @alexpott on IRC and this should be moved to 8.1.x. We should also add an update path.

Re-roll: This great patch doesn't apply to RC3 anymore, so here is a re-roll. No actual changes to #114 has been made.

Go testbot.

Ok i get it. We don't have automated tests for 8.1.x.

I have made a new patch. The access was right but some permissions like "delete term" only worked when called over url and not over the UI. The following changes has been made:

• Only vocabularies where the user has the write to add a term is show in the overview
• The Term overview pages can also be access if the user have the new "Access the taxonomy vocabulary overview page" permission since. Otherwise user get an "access denied" when clicking "list terms" on vocabulary overview page.

The suggestions from #116 has also been Incorporated, except 5.

1. +++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
   @@ -46,11 +96,14 @@ public function getDefaultOperations(EntityInterface $entity) {
   +    if ($taxonomy_term_access_control_handler->createAccess($entity->id())) {
   

   The list builder class has a current user, pass it on to the access control handler.

2. +++ b/core/modules/taxonomy/src/VocabularyListBuilder.php
   @@ -68,8 +126,11 @@ public function buildHeader() {
   +    if ($taxonomy_term_access_control_handler->createAccess($entity->id())) {
   

   Invoke createAccess with $this->current_user.

New patch: changes has been made to the logic on taxonomy page. More similar to original patch. Instead of removing the row with vocabularies where the user didn't have the right to create new terms, all vocabularies are shown with the minimum of a "view terms" button.

Hi! Thanks everyone for contributing to this issue. I'm a strong supporter of Taxonomy Access Fix so I really hope that this could be integrated in a future release of the Core.

First, I'd like to give my 2 cts about the last patch :

1. +++ b/core/modules/taxonomy/taxonomy.routing.yml
   @@ -4,7 +4,7 @@ entity.taxonomy_vocabulary.collection:
   -    _permission: 'administer taxonomy'
   +    _access_taxonomy_overview: 'TRUE'
   
   @@ -72,7 +72,7 @@ entity.taxonomy_vocabulary.overview_form:
   -    _entity_access: 'taxonomy_vocabulary.view'
   +    _access_taxonomy_overview: 'TRUE'
   

   Do we really need to declare an access service there?
   Couldn't we just use _permission: 'administer taxonomy+access taxonomy overview' ?

2. +++ b/core/modules/taxonomy/taxonomy.services.yml
   @@ -4,3 +4,7 @@ services:
   diff --git a/core/modules/views/src/Tests/Wizard/WizardTestBase.php b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
   
   diff --git a/core/modules/views/src/Tests/Wizard/WizardTestBase.php b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
   index 7d8d9c0..0e35233 100644
   
   index 7d8d9c0..0e35233 100644
   --- a/core/modules/views/src/Tests/Wizard/WizardTestBase.php
   
   --- a/core/modules/views/src/Tests/Wizard/WizardTestBase.php
   +++ b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
   
   +++ b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
   +++ b/core/modules/views/src/Tests/Wizard/WizardTestBase.php
   @@ -15,6 +15,13 @@
   
   @@ -15,6 +15,13 @@
    abstract class WizardTestBase extends ViewTestBase {
    
      /**
   +   * Permissions for this user to test with.
   +   *
   +   * @var array
   +   */
   +  protected $permissions = [];
   +
   +  /**
       * Modules to enable.
       *
       * @var array
   @@ -24,8 +31,10 @@
   
   @@ -24,8 +31,10 @@
      protected function setUp() {
        parent::setUp();
    
   +    $this->permissions = array_merge($this->permissions, ['administer views', 'administer blocks', 'bypass node access', 'access user profiles', 'view all revisions']);
   +
        // Create and log in a user with administer views permission.
   -    $views_admin = $this->drupalCreateUser(array('administer views', 'administer blocks', 'bypass node access', 'access user profiles', 'view all revisions'));
   +    $views_admin = $this->drupalCreateUser($this->permissions);
        $this->drupalLogin($views_admin);
        $this->drupalPlaceBlock('local_actions_block');
      }
    
   

   All this part looks out of scope.

-----

Then I'd like to elaborate on the upgrade path. We absolutely need one and we need to define the mapping between the current permissions and the new ones. This is my proposal:

-----

One last thought, in \Drupal\taxonomy\TermAccessControlHandler::checkAccess() the 'access content' permission is used to check access for the 'view' operation. Shouldn't we add a new permission per vocabulary to handle this case?

Thanks @DuaelFr for the review!

Just tried out the patch. It seems the access checking for the OverviewTerms is not working correctly. For a user with only the access taxonomy overview permission (but not administer taxonomy) the weight fields and operations get properly hidden, but the "Save" and "Reset to alphabetical" buttons are still shown and the latter even works, which is not nice.

I don't agree.
It's really easy to provide an upgrade path for this kind of issue and, if necessary, we could keep the old permission machine name in case some contrib rely on it.

I'm confused about consistency in naming.

The whole comments mention a "create term" permission and suddenly the permission label is "Add term"?
Note that for node creation, the permission uses the label "Create" while the effective button is labeled "Add content" and all other UI mentions.
There is only a single permission related to "Add" that is field.
So i think we should name it "Create term" and create a follow-up to switch consistently to Add content / add term, ...

(Note to self: don't try to add related issues while on mobile.)

Rerolling, let's continue work on this!

Fixed conflict on core/modules/taxonomy/src/Form/OverviewTerms.php

Setting it to needs review to see what the bot says, but this patch still needs work based on #125, #127 and #131.

I did the changes suggested in #125 and #127. I attached a screenshot of the terms overview page after the change (the "Save" and "Reset to alphabetical" buttons are gone with access taxonomy overview permission only (shouldn't we also remove the message above, about re-ordering?)). As for #131, despite the entire discussion going around a "Add terms" permission, the permission does read "Tags: Create terms" on /admin/people/permissions (screenshot attached). The button on /admin/structure/taxonomy/manage/tags/overview does read "Add term". So the inconsistency seems... properly preserved ! Also, the text on the terms overview page state to use the drag-and-drop handles even if there's only 1 element (and thus no handles), this should probably be fixed (screenshot attached)?

Looking great. A bit more feedback.

The inconsistency about "Add" in #131 was fixed in #135. Pity @Manuel Garcia did neither provide an interdiff, nor list the items he fixed. I think the situation is fine (aka consistently inconsistent ;-D)

Yeah the intro text in the term overview is wrong now. It still states dragging although you don't have permissions.

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -199,14 +207,23 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
       '#header' => array($this->t('Name'), $this->t('Weight'), $this->t('Operations')),

The weight column is still unconditionally defined in the header?

@miro_dietiker my patch on #135 is just a reroll of #124, I did not fix any issues mentioned here, no need for a interdiff.

All i did was fix a simple merge conflict on core/modules/taxonomy/src/Form/OverviewTerms.php, which was the reason why we had to reroll the patch.

Note also that now the intro text in the term overview is wrong. It still states dragging although you don't have permissions!

I changed this in taxonomy_help(), while keeping the different phrasings, dependently of the terms hierarchy.

The weight column is still unconditionally defined in the header?
That line looks pointless, actually. If we look at :

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -313,44 +342,54 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+    $form['terms']['#header'] = array($this->t('Name'));

$form['terms']['#header'] is overwritten here and never used after the first declaration. Then, the Weight column is properly added :

+++ b/core/modules/taxonomy/src/Form/OverviewTerms.php
@@ -313,44 +342,54 @@ public function buildForm(array $form, FormStateInterface $form_state, Vocabular
+    if ($change_weight_access) {
+      $form['terms']['#header'][] = $this->t('Weight');

I removed it. No change in UI when testing locally.

This would be great to get into Drupal 8.3, or is there a contrib module which accomplishes the same thing?

EDIT: To answer my own question - Yes, and it works fine: Taxonomy Access Fix.

Rerolling last patch...

(8.3.x) $ git apply --index split_administer-1848686-139.patch 
error: patch failed: core/modules/taxonomy/src/Form/OverviewTerms.php:313
error: core/modules/taxonomy/src/Form/OverviewTerms.php: patch does not apply
error: patch failed: core/modules/taxonomy/src/VocabularyListBuilder.php:56
error: core/modules/taxonomy/src/VocabularyListBuilder.php: patch does not apply
error: patch failed: core/modules/taxonomy/taxonomy.module:66
error: core/modules/taxonomy/taxonomy.module: patch does not apply

Manually fixed conflicts in:

• core/modules/taxonomy/src/VocabularyListBuilder.php
• core/modules/taxonomy/src/Form/OverviewTerms.php

The rebase did the rest.

I couldn't get #143 to apply on 8.2.3. New patch in attachment.

To trigger test on the latest patch

@Algeron Thanks for working on this, but the patch on #143 is against 8.3.x, and still applies cleanly. We have to do this on 8.3.x not 8.2.x.

Patch in #143 still applies cleanly against 8.4.x.

#143 doesn't apply against 8.3.0-rc1.

Attached does.

previous was missing one file..

#143 doesn't apply against 8.3.0-rc1.

Attached does.

Am I correct in assuming #153/#153 is just intended to be able to apply this patch to 8.3 for anyone who is actively using this in their projects? It would need to apply cleanly to the current HEAD, which is 8.4.

@eelkeblok try the patch in #143

I don't need the patch, I'm just trying to understand the nature of the patch from #153. At some point this will need to be applied in the project Git and the committer will need to understand what patch to use.

Setting back to needs work. It now looks like #143 does not, in fact, apply to 8.4 (see test results).

Rerolling...

Here is a reroll of #143 against 8.4.x

Manually fixed on:

• VocabularyPermissionsTest.php
• VocabularyListBuilder.php
• OverviewTerms.php
• NodeAccessBaseTableTest.php

Patch #161 do not apply anymore on 8.4.x.

Here is a fixed one.

This should fix those failing tests

Should fix remaining failed tests.

Here is a new patch with code standard fixes.
And a Backport for 8.3 fixed.

Reroll