Support from Acquia helps fund testing for Drupal Acquia logo

Comments

webchick’s picture

Talked about this with Wim some. It's a known issue, and related to #2010930: [META] Apply formatters and widgets to rendered entity base fields (all besides node.title) but will have a different solution, so we decided to keep this one open.

Chat excerpt:

Wim Leers
we *did* make the title in-place editable, and that *does* still work. (Just tested + confirmed.) There's one huge caveat. Drupal has this crazy crazy crazy thing where if you're looking at "the full entity page" (e.g. node/5 or taxonomy/term/3), then the title is *removed* from the entity display, and is "promoted" to the *page* title. And then the necessary metadata is missing of course, hence breaking in-place editing.
Therefore, title in-place editing works everywhere except on "the full entity page".

Angela Byron
10:29 Wim Leers: ugh. can we fix that?
10:29 Cos as an end user it just looks busted

Wim Leers
Angela Byron: it's very very very difficult
10:29 Angela Byron: I know
Angela Byron: and so far, it's AFAIK impossible to set additional attributes on page title

Wim Leers’s picture

Title: Can no longer in-place edit node titles on full node view » Entity labels are not in-place editable on "full entity page" (prime example: node title)
Component: edit.module » entity system
Assigned: Unassigned » Wim Leers
Status: Active » Needs review
Issue tags: +Spark, +sprint, +DrupalWTF
FileSize
6.07 KB

Tests still needed, but this fixes it — generically, for all entities. Major thanks to plach & Berdir!

Wim Leers’s picture

Issue tags: +Entity Field API

Status: Needs review » Needs work

The last submitted patch, 2: entity_full_page_label_in_place-2216437-2.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
1.72 KB

Fix fatal.

Status: Needs review » Needs work

The last submitted patch, 5: entity_full_page_label_in_place-2216437-5.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.74 KB
2.45 KB

Now Edit's JS is also smart enough to initially position itself against the title element (in fact, whichever the topmost element is).

Status: Needs review » Needs work

The last submitted patch, 7: entity_full_page_label_in_place-2216437-5.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
7.67 KB

Chasing HEAD.

Status: Needs review » Needs work

The last submitted patch, 9: entity_full_page_label_in_place-2216437-9.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
8.92 KB

#9 rerolled #5 instead of #7…

#wimfail

Status: Needs review » Needs work

The last submitted patch, 11: entity_full_page_label_in_place-2216437-11.patch, failed testing.

dawehner’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
    @@ -61,9 +62,21 @@ public static function create(ContainerInterface $container) {
    +
    +    // If the entity's label is rendered using a field formatter, set that as
    +    // the page title instead, so that attributes set on the field propagate
    +    // correctly (e.g. RDFa, in-place editing).
    +    if ($_entity instanceof ContentEntityInterface) {
    +      $label_field = $_entity->getEntityType()->getKey('label');
    +      if ($label_field && $_entity->getFieldDefinition($label_field)->getDisplayOptions('view')) {
    +        $page['#title'] = drupal_render($page[$label_field], TRUE);
    +      }
    +    }
    +
    +    return $page;
       }
    

    An explicit webtest for this would be helpful.

  2. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeViewController.php
    @@ -0,0 +1,56 @@
    +/**
    + * Defines a generic controller to render a single entity.
    + */
    +class NodeViewController extends EntityViewController {
    

    Let's adapt the documentation a bit

  3. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeViewController.php
    @@ -0,0 +1,56 @@
    +   * @param \Drupal\node\EntityInterface $node
    ...
    +  public function view(EntityInterface $node, $view_mode = 'full', $langcode = NULL) {
    

    Why do we use the generic EntityInterface in contrast to NodeInterface in the previous patches? All the code requires nodes anyway

  4. +++ b/core/modules/node/node.routing.yml
    @@ -47,7 +47,7 @@ node.add:
    -    _content: '\Drupal\node\Controller\NodeController::page'
    +    _content: '\Drupal\node\Controller\NodeViewController::view'
         _title_callback: '\Drupal\node\Controller\NodeController::pageTitle'
       requirements:
    

    Should we also move the title_callback onto the new class?

Wim Leers’s picture

Status: Needs work » Needs review
Issue tags: -Needs tests
FileSize
13.25 KB
5.46 KB
  1. Yep, of course — just wanted to get it green :) Now comes with tests. Including an assertion to ensure that in-place editing on the node title continues to work correctly.
  2. Fixed.
  3. Because using NodeInterface instead causes a strict warning: Strict warning: Declaration of Drupal\node\Controller\NodeViewController::view() should be compatible with Drupal\Core\Entity\Controller\EntityViewController::view(Drupal\Core\Entity\EntityInterface $_entity, $view_mode = 'full', $langcode = NULL).
  4. I think that would be sensible, but I'd rather not do it in this patch. Because e.g. the user module does use EntityViewController but also still specifies its own title callback. Fixing it here implies fixing it there, and then the scope grows

Last test failures fixed.

Status: Needs review » Needs work

The last submitted patch, 14: entity_full_page_label_in_place-2216437-14.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.1 KB
1.91 KB
dawehner’s picture

Sorry but I don't see the point. The title callback was easy to find previously, because it was simply in the same file. Well, I cannot RTBC this patch anyway as it contains javascript changes.

Wim Leers’s picture

Issue tags: +JavaScript

Good point regarding JavaScript. Tagging as such, to get sign-off from e.g. nod_.

What don't you see the point of?

Wim Leers’s picture

Wim Leers’s picture

Critical because blocking the critical #2091401: Add hook_help for Quick Edit module.

Wim Leers’s picture

Priority: Major » Critical
Wim Leers’s picture

Issue tags: +beta target
xjm’s picture

Issue tags: -beta target

Not really related to the beta criteria, so untagging. Let's reach out to specific folks for reviews.

Gábor Hojtsy’s picture

Just reviewed this. First, hats off! I think its a testament to the new rendering and the page system that this can be implemented with relatively small amount of code specifically crafted for the feature. Wow.

  1. +++ b/core/modules/edit/lib/Drupal/edit/Tests/EditLoadingTest.php
    @@ -299,6 +299,10 @@ public function testTitleBaseField() {
    +    $this->assertRaw('<h1><span class="field field-node--title field-name-title field-type-string field-label-hidden" data-edit-field-id="node/1/title/und/full">' . $node->label() . '</span></h1>');
    

    This is quite a bit too specific on the theme. Should we just assert on an xpath that an element with this data-edit-field-id exists and has the expected title? Or would that be not enough to ensure the editability?

  2. +++ b/core/modules/entity/lib/Drupal/entity/Tests/EntityDisplayTest.php
    @@ -42,9 +42,10 @@ public function testEntityDisplayCRUD() {
         // Check that providing no 'weight' results in the highest current weight
    -    // being assigned.
    -    $expected['component_1'] = array('weight' => 0);
    -    $expected['component_2'] = array('weight' => 1);
    +    // being assigned. The 'name' field's formatter has weight -5, therefore
    +    // these follow.
    +    $expected['component_1'] = array('weight' => -4);
    +    $expected['component_2'] = array('weight' => -3);
         $display->setComponent('component_1');
         $display->setComponent('component_2');
         $this->assertEqual($display->getComponent('component_1'), $expected['component_1']);
    @@ -63,6 +64,12 @@ public function testEntityDisplayCRUD() {
    
    @@ -63,6 +64,12 @@ public function testEntityDisplayCRUD() {
         }
     
         // Check that getComponents() returns options for all components.
    +    $expected['name'] = array(
    +      'label' => 'hidden',
    +      'type' => 'string',
    +      'weight' => -5,
    +      'settings' => array(),
    +    );
         $this->assertEqual($display->getComponents(), $expected);
     
         // Check that a component can be removed.
    @@ -164,7 +171,7 @@ public function testFieldComponent() {
    
    @@ -164,7 +171,7 @@ public function testFieldComponent() {
         $default_formatter = $field_type_info['default_formatter'];
         $formatter_settings =  \Drupal::service('plugin.manager.field.formatter')->getDefaultSettings($default_formatter);
         $expected = array(
    -      'weight' => 0,
    +      'weight' => -4,
           'label' => 'above',
    

    Its not clear to me how these changes are related?

  3. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeViewController.php
    @@ -0,0 +1,56 @@
    + * Contains \Drupal\node\Controller\NodeViewController.
    

    Nice that this is now its own controller :)

Because the review points are not necessarily things to be changed, not moving the issue status.

Wim Leers’s picture

Thanks for the review!

  1. Lots of assertions are this specific, because all themes use the Stark theme? This could be changed to an XPath selector, just not sure if it's worth it.
  2. The title was not a component before, but a hardcoded thing. Now that the title *is* a component, we need to adjust the tests a little bit.
  3. :)
Gábor Hojtsy’s picture

While that full markup matching something I have seen elsewhere indeed, I've also seen xpath matching used much. Anyway, I don't want to hold back a critical bugfix on that.

I also looked into @dawehner's concern. The title callback here used to be on Node's on controller, from where the new view controller was born. I don't think moving over the title callback for the view page to the Node view controller implies needing to fix it for the user view controller either, which is what @dawehner means AFAIS. The view code is moved anyway and this patch works with the title :) Not seeing how that would not be in scope then.

Also still needs JS review :)

dawehner’s picture

Lots of assertions are this specific, because all themes use the Stark theme? This could be changed to an XPath selector, just not sure if it's worth it.

If you agree that relying on the HTML structure might cause issues in the future it IS always worth to put any effort in NOW when we know that a problem might exist.
Figuring out the problem later is always a order of magnitude harder.

I also looked into @dawehner's concern. The title callback here used to be on Node's on controller, from where the new view controller was born. I don't think moving over the title callback for the view page to the Node view controller implies needing to fix it for the user view controller either, which is what @dawehner means AFAIS. The view code is moved anyway and this patch works with the title :) Not seeing how that would not be in scope then.

My point is simple: We had the related code in a similar place before, with your patch we now longer have. It is as simple as that.

Wim Leers’s picture

All concerns addressed.

Status: Needs review » Needs work

The last submitted patch, 28: entity_full_page_label_in_place-2216437-16.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
Gábor Hojtsy’s picture

Can you post an interdiff of the changes for easier review? Thanks!

Status: Needs review » Needs work

The last submitted patch, 28: entity_full_page_label_in_place-2216437-16.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
15.69 KB
4.18 KB

I moronically uploaded the wrong patch in #28 — I simply reuploaded the same patch as in #16 :D

Here it is again, with interdiff this time!

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Looks great! Thanks for resolving those concerns.

catch’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/lib/Drupal/Core/Entity/Controller/EntityViewController.php
    @@ -61,9 +62,21 @@ public static function create(ContainerInterface $container) {
    +    // the page title instead, so that attributes set on the field propagate
    

    The comment could be more specific. Instead of what? (also see below)

    Also won't this break once #pre_render caching is in? $page can't be guaranteed to hold $label_field then, it'll just be #cache and #pre_render etc.

    I think this needs a $entity->$label_field->view() so that we really are just rendering the field itself.

  2. +++ b/core/modules/node/lib/Drupal/node/Controller/NodeViewController.php
    @@ -0,0 +1,64 @@
    +    return String::checkPlain($this->entityManager->getTranslationFromContext($node)->label());
    

    Why setting up the title here, then overridding when the page is viewed. Is it because we can't do the full formatter render here? Could use a comment I think as to why we set the page title twice.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
16.02 KB
1.68 KB

#35:

    • RE: more specific comment: done.
    • RE: #pre_render caching: good point. Fixed.
  1. I've wondered the same, but AFAICT _title_callback may be used in non-HTML circumstances as well: to get the plain text title for listings, for REST calls, and whatnot. OTOH, the $page['#title'] mechanism allows us to specify a "rich HTML title" in the case where we know it's going to be a HTML page.
    Also: we don't have access to the view mode in NodeViewController::title, which is essential.

Status: Needs review » Needs work

The last submitted patch, 36: entity_full_page_label_in_place-2216437-36.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
16.63 KB
1.54 KB

Test failures were due to simple whitespace mismatch, and a lack of translation introduced by #36.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good. Still looks ready as per our prior reviews.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 38: entity_full_page_label_in_place-2216437-38.patch, failed testing.

Wim Leers’s picture

Status: Needs work » Needs review
FileSize
19.52 KB
2.97 KB

Ugh, one new fail. In Drupal\entity_reference\Tests\EntityReferenceFormatterTest, which didn't exist previously, hence the new fail.

The fail happens because there is now a new field formatter being rendered. Adjusted the tests to cope with that, and minimized the number of changes.

Gábor Hojtsy’s picture

Status: Needs review » Reviewed & tested by the community

While (as discussed above) I don't think the so exact (to the whitespace) rendering test is dangerous and prone to fail on unrelated things (as it did here :), I see the changed test already does that. Let's fix it then and get this critical done finally.

nod_’s picture

Js is ok with me.

catch’s picture

Status: Reviewed & tested by the community » Fixed

That's reasonable on _title_callback.

Committed/pushed to 8.x, thanks!

  • Commit 209ef5e on 8.x by catch:
    Issue #2216437 by Wim Leers: Entity labels are not in-place editable on...
Wim Leers’s picture

Issue tags: -sprint

YAY! :)

Status: Fixed » Closed (fixed)

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