Problem/Motivation

quickedit_preprocess_page_title(&$variables) does not check whether the current user has access in-place editing permission, thus resulting in having the class show in the markup even when it shouldn't.

Proposed resolution

Check whether the current user has access in-place editing permission in order to add the class.

Remaining tasks

Do it.

User interface changes

None.

API changes

None.

Data model changes

None.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

thpoul created an issue. See original summary.

thpoul’s picture

Title: Don't append js-quickedit-page-title class when user has no 'access in-place editing' persmission » Don't append 'js-quickedit-page-title' class when user doesn't have 'access in-place editing' permission
Status: Active » Needs review
FileSize
598 bytes

And here is the patch.

darketaine’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
83.22 KB
50.94 KB
33.06 KB
81.82 KB

I confirm the problem and that the patch solves it without creating problem when the user has permission for in-place editing.

I attach screenshots before (both with and without in-place editing permission) and after

catch’s picture

Status: Reviewed & tested by the community » Needs work

Given the preprocess is currently a one-liner, let's avoid the negative condition and early return, and add the class inside the if instead.

darketaine’s picture

Status: Needs work » Needs review
FileSize
662 bytes
754 bytes

Here is the patch according to #4

thpoul’s picture

Status: Needs review » Reviewed & tested by the community

Thank you @catch & @darketaine, back to RTBC.

alexpott’s picture

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

Lucky permissions are a permanent cache context otherwise this sort of variability in a template is tricky. How given this is a condition on a permission we should test it.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Wim Leers’s picture

Priority: Normal » Minor
Issue tags: +TX (Themer Experience)
Wim Leers’s picture

Issue tags: +Novice, +php-novice

We can easily add two simple assertions to \Drupal\quickedit\Tests\QuickEditLoadingTest::testUserWithoutPermission() and \Drupal\quickedit\Tests\QuickEditLoadingTest::testUserWithPermission().

omitsis’s picture

Version: 8.2.x-dev » 8.3.x-dev
Assigned: Unassigned » omitsis
Status: Needs work » Needs review
FileSize
1.39 KB
2.04 KB

OK. Go with tests!

The last submitted patch, 11: 2787471-tests-should-fail.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 11: 2787471-tests-fix.patch, failed testing.

omitsis’s picture

Oh sorry! Now with correct xpath.

The last submitted patch, 14: 2787471-14-tests-should-fail.patch, failed testing.

omitsis’s picture

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community

Test fail, fix pass.

+    $this->assertNoFieldByXPath('//h1[contains(@class, "js-quickedit-page-title")]', NULL, 'in-place js-quickedit-page-title editing class does not exists');

Point at the end of the sentence, can be fixed on commit.

Wim Leers’s picture

Wim Leers’s picture

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

Tests have been added.

  1. +++ b/core/modules/quickedit/quickedit.module
    @@ -117,7 +117,9 @@ function quickedit_field_formatter_info_alter(&$info) {
    -  $variables['title_attributes']['class'][] = 'js-quickedit-page-title';
    +  if (\Drupal::currentUser()->hasPermission('access in-place editing')) {
    +    $variables['title_attributes']['class'][] = 'js-quickedit-page-title';
    +  }
    

    This is missing a cache context. See the patch at #2528498-23: Only emit Quick Edit data- attributes when actually necessary.

  2. +++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
    @@ -108,6 +108,9 @@ public function testUserWithoutPermission() {
    +    // Check that does not exist in-place editing title class.
    

    This sentence does not make sense. Let's use the same sentence as #2528498-23: Only emit Quick Edit data- attributes when actually necessary uses.

  3. +++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
    @@ -108,6 +108,9 @@ public function testUserWithoutPermission() {
    +    $this->assertNoFieldByXPath('//h1[contains(@class, "js-quickedit-page-title")]', NULL, 'in-place js-quickedit-page-title editing class does not exists');
    
    @@ -161,6 +164,9 @@ public function testUserWithPermission() {
    +    $this->assertFieldByXPath('//h1[contains(@class, "js-quickedit-page-title")]', NULL, 'in-place editing js-quickedit-page-title class exists.');
    

    Let's omit the messages here, because they don't make a lot of sense (and adding such messages doesn't add much value anyway).

  4. +++ b/core/modules/quickedit/src/Tests/QuickEditLoadingTest.php
    @@ -161,6 +164,9 @@ public function testUserWithPermission() {
    +    // Check for in-place editing title class.
    

    Let's use the same sentence as #2528498-23: Only emit Quick Edit data- attributes when actually necessary uses.

omitsis’s picture

Status: Needs work » Needs review
FileSize
2.56 KB

Maybe we can merge two comments in one? :)

oriol_e9g’s picture

Status: Needs review » Reviewed & tested by the community

Go again.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed 10210c7 and pushed to 8.3.x. Thanks!

  • alexpott committed 10210c7 on 8.3.x
    Issue #2787471 by omitsis, darketaine, thpoul, Wim Leers: Don't append '...

  • alexpott committed 10210c7 on 8.4.x
    Issue #2787471 by omitsis, darketaine, thpoul, Wim Leers: Don't append '...

  • alexpott committed 10210c7 on 8.4.x
    Issue #2787471 by omitsis, darketaine, thpoul, Wim Leers: Don't append '...

Status: Fixed » Closed (fixed)

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