Layout page does not indicate which View Mode is being edited

Fix Cc Issue #16 && Attached patch against Drupal 10.1.x

I think this might need different approach to fix.
Apparently, `LayoutBuilderRoutesTrait` is calling same `title()` method for general layout builder page like "node/nid/layout" and "admin/structure/types/manage/{type}/display/{display_mode}layout"

This is causing multiple test case failures. Let me know if we need to add additional tests.
I've added a separate method altogether to override the title when we are on default override page.

I've added simple test case to validate the title change. Also, I checked random 5 failing test cases from #20 and those as passing as well.

The last submitted patch, 22: 3008923-22.patch, failed testing. View results →

Fixing test case failures now.
DefaultsSectionStorageTest unit test are passing on local.
It was failing because we added a new method.

Applied patch #24
On a standard install of 10.1 enabled layout builder
Enabled layout builder for Articles and when I went to manage layout title said Edit Default layout for Article content items Add to Default shortcuts

Tested with taxonomy too (never used layout builder with taxonomy) and title said "Edit Default layout for Tags taxonomy terms"

Wasn't sure about the empty constructor but see other instances in core of it so assume it's fine.

A couple of problems with the current implementation, and the last bullet suggests a new direction.

1. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -12,24 +16,62 @@
   +  public function __construct(protected EntityDisplayRepositoryInterface $entityDisplayRepository) {
   +  }
   

   If we're going with constructor property promotion, let's do it the "good" way:
   put the property on its own line, with a trailing comma

2. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -12,24 +16,62 @@
   -  public function title(SectionStorageInterface $section_storage) {
   -    assert(Inspector::assertStringable($section_storage->label()), 'Section storage label is expected to be a string.');
   +  public function title(SectionStorageInterface $section_storage): TranslatableMarkup {
   

   Not sure why the assert is being removed

3. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -12,24 +16,62 @@
   +    [$entity_type, $bundle, $view_mode] = explode('.', $section_storage->getStorageId());
   

   This is not guaranteed to be the structure of the storage ID.

   You should be able to use $section_storage->getContextValue('display') to get the entity view display.

4. +++ b/core/modules/layout_builder/src/Routing/LayoutBuilderRoutesTrait.php
   @@ -64,6 +64,9 @@ protected function buildLayoutRoutes(RouteCollection $collection, SectionStorage
   +    if ($type === 'defaults') {
   +      $main_defaults['_title_callback'] = '\Drupal\layout_builder\Controller\LayoutBuilderController::overriddenSectionTitle';
   +    }
   

   There shouldn't be any $type specific code in this trait.

   This can be modified directly in DefaultsSectionStorage::buildRoutes(), see the other changes made to the collection after the call to buildLayoutRoutes()

5. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -12,24 +16,62 @@
        return $this->t('Edit layout for %label', ['%label' => $section_storage->label() ?? $section_storage->getStorageType() . ' ' . $section_storage->getStorageId()]);
   ...
   +    return $this->t('Edit %view_mode layout for %label', [
   +      '%view_mode' => $view_mode,
   +      '%label' => $section_storage->label(),
   +    ]);
   

   Upon reflection, maybe we should add a getEditLabel() method to SectionStorageInterface? We can move the current implementation to SectionStorageBase, and have LayoutBuilderController::title delegate to that method

Fixing the feedback mentioned in #26

Based on 5th point, I've added a new method and included other changes during that refactoring.
1st and 4th point are not required anymore.
2nd and 3rd one are fixed in refactoring.

Both the test cases are passing now, will see if I came across new failures post refactoring.

With the new parameter to DefaultsSectionStorage will it not have to default to null and trigger a warning it will be needed in 11?

I think we might need that. I missed that.
If there is a class in contrib/custom modules which extends DefaultsSectionStorage might break.

- Added a service in BC compatible way
- Add a test case for the same as well.

1. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -22,12 +22,11 @@ class LayoutBuilderController {
   -    return $this->t('Edit layout for %label', ['%label' => $section_storage->label() ?? $section_storage->getStorageType() . ' ' . $section_storage->getStorageId()]);
   ...
   +    return $section_storage->getEditLabel();
   

   Much cleaner 👍🏻

2. +++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
   @@ -22,12 +22,11 @@ class LayoutBuilderController {
   +  public function title(SectionStorageInterface $section_storage): TranslatableMarkup {
   

   Side note: Are we really doing this now?

3. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -407,4 +423,18 @@ public function setContext($name, ComponentContextInterface $context) {
   +    $display = $this->getContextValue('display');
   

   This could now be $display = $this->getSectionList();, not a big deal either way

4. +++ b/core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php
   @@ -407,4 +423,18 @@ public function setContext($name, ComponentContextInterface $context) {
   +    assert($display instanceof LayoutBuilderEntityViewDisplay);
   

   This seems overkill. And could be not true. And doesn't really need to be?
   If you insist, let's check what is necessary: \Drupal\Core\Entity\Display\EntityDisplayInterface

5. +++ b/core/modules/layout_builder/tests/src/Unit/DefaultsSectionStorageTest.php
   @@ -50,6 +51,13 @@ class DefaultsSectionStorageTest extends UnitTestCase {
   +  /**
   +   * The entity display repository service.
   +   *
   +   * @var \Drupal\Core\Entity\EntityDisplayRepositoryInterface
   +   */
   +  protected $entityDisplayRepository;
   
   @@ -59,12 +67,13 @@ protected function setUp(): void {
        $entity_type_bundle_info = $this->prophesize(EntityTypeBundleInfoInterface::class);
   ...
   +    $this->entityDisplayRepository = $this->prophesize(EntityDisplayRepositoryInterface::class);
   

   Nit: like $entity_type_bundle_info, this property is not being mocked further on in the method, so it can live as a local variable

For the points in #31.

Please address or attempt to address all points and not just one or two.

Hi @tim.plunkett
I've addressed all the issues except the second one.

+++ b/core/modules/layout_builder/src/Controller/LayoutBuilderController.php
@@ -22,12 +22,11 @@ class LayoutBuilderController {
+  public function title(SectionStorageInterface $section_storage): TranslatableMarkup {

Side note: Are we really doing this now

Do you mean that we should directly call $this->getEditLabel() in LayoutBuilderRoutesTrait instead of providing title_callback of the controller?

I tried to do that on local and was receiving following error on the test case:

Drupal\Component\Plugin\Exception\ContextException : The 'entity' context is required and not present.
 /data/app/core/lib/Drupal/Core/Plugin/Context/Context.php:73
 /data/app/core/lib/Drupal/Core/Plugin/ContextAwarePluginTrait.php:82
 /data/app/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php:136
 /data/app/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php:326
 /data/app/core/modules/layout_builder/src/Plugin/SectionStorage/SectionStorageBase.php:122
 /data/app/core/modules/layout_builder/src/Routing/LayoutBuilderRoutesTrait.php:66
 /data/app/core/modules/layout_builder/src/Plugin/SectionStorage/OverridesSectionStorage.php:237

It seems to me that entity is not initialised that when we are building routes.

Re #31.2 I meant specifying the return type as TranslatableMarkup for a method that could just as easily return a string. Ignore that gripe, sorry for the confusion

+++ b/core/modules/layout_builder/tests/src/Kernel/DefaultsSectionStorageTest.php
@@ -222,4 +225,17 @@ public function testLoadFromDisplay() {
+  public function testRequiredServiceTriggerError() {

Nit: this should now specify void as the return type

The rest of the changes look good!

Sounds good then.
Keeping the TranslatableMarkup return type as it is and adding void return type in test method.

Think all that's needed is a change record now and that to be added to the trigger_error. So people know when this was introduced.

- Added a change record here https://www.drupal.org/node/3356831 →
- Fixed one more issue spotted related to usage of DI
- Updated the patch.

One small spot. In the trigger_error can you add the change record.

Can self RTBC after that as everything else looks good to me.

Actually it was so small I didn't mind making the change.

I'm not certain that the proposed text is entirely correct because it's using layout as the noun for what is usually called a view mode. Shouldn't the sentence be something along the lines of "Edit layout for [view_mode] view mode for Article content items".

With this change, the title becomes pretty long, meaning it might be worthwhile investigating if there's a better way for us to indicate the view mode that is being edited. I wonder if this should be indicated at least in the breadcrumb? Updating the title to be more generic to indicate that we can look into solutions beyond changing the title of the page.

See what you mean.

But your suggestion, yes it's long, but think it makes the most sense.
1. Tells me I'm in layout
2. Tells me the view mode
3. Tells me the content type
4. Tells me it applies to all content items.

So even though it's long I think it hits all the points of what is being done. And it's an edit page so more info the better

But if we are against the long title

What if we add the view mode to the description block where "This layout builder tool allows you to configure the layout of the main content area." is?

Thanks for the patch. I tried to test the changes available in the patch with the standard configuration. Applied the patch #41 🐛 Layout page does not indicate which View Mode is being edited Needs work with Drupal 10.1.0-dev.

I am getting the error "The website encountered an unexpected error. Please try again later" on the default display layout page.
I got the below error from php logs.

Error: Call to a member function getViewModeOptionsByBundle() on null in Drupal\layout_builder\Plugin\SectionStorage\DefaultsSectionStorage->getEditLabel() (line 430 of core/modules/layout_builder/src/Plugin/SectionStorage/DefaultsSectionStorage.php).

Steps to reproduce
1. Install Drupal with the standard profile.
2. Enable Layout Builder on Article content type.
3. Go to Manage Layout on Default view mode at /admin/structure/types/manage/article/display/default/layout.
4. I got the above error.

Tagging for usability review based on feedback from #42 adn #43

Please, re-roll patch against 10.3.x.

I wish I could do it my self.... some day...

Thank you in advance !