- 🇮🇳India Nikhil_110
Fix Cc Issue #16 && Attached patch against Drupal 10.1.x
- Status changed to Needs review
over 1 year ago 10:34am 14 April 2023 - 🇮🇳India mohit_aghera Rajkot
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 →
- 🇮🇳India mohit_aghera Rajkot
Fixing test case failures now.
DefaultsSectionStorageTest
unit test are passing on local.
It was failing because we added a new method. - Status changed to RTBC
over 1 year ago 10:27pm 15 April 2023 - 🇺🇸United States smustgrave
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 shortcutsTested 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.
- Status changed to Needs work
over 1 year ago 9:15pm 16 April 2023 - 🇺🇸United States tim.plunkett Philadelphia
A couple of problems with the current implementation, and the last bullet suggests a new direction.
-
+++ 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 -
+++ 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
-
+++ 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. -
+++ 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 tobuildLayoutRoutes()
-
+++ 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
-
- Status changed to Needs review
over 1 year ago 5:47am 17 April 2023 - last update
over 1 year ago Custom Commands Failed - 🇮🇳India mohit_aghera Rajkot
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.
- last update
over 1 year ago 29,206 pass - Status changed to Needs work
over 1 year ago 2:32pm 19 April 2023 - 🇺🇸United States smustgrave
With the new parameter to DefaultsSectionStorage will it not have to default to null and trigger a warning it will be needed in 11?
- Status changed to Needs review
over 1 year ago 1:11pm 24 April 2023 - last update
over 1 year ago 29,305 pass - 🇮🇳India mohit_aghera Rajkot
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. - 🇺🇸United States tim.plunkett Philadelphia
-
+++ 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 👍🏻
-
+++ 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?
-
+++ 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 -
+++ 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
-
+++ 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
-
- Status changed to Needs work
over 1 year ago 10:25pm 24 April 2023 - 🇺🇸United States smustgrave
For the points in #31.
Please address or attempt to address all points and not just one or two.
- 🇮🇳India mohit_aghera Rajkot
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()
inLayoutBuilderRoutesTrait
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.
- Status changed to Needs review
over 1 year ago 12:00pm 26 April 2023 - last update
over 1 year ago 29,344 pass - 🇺🇸United States tim.plunkett Philadelphia
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 typeThe rest of the changes look good!
- last update
over 1 year ago 29,344 pass - 🇮🇳India mohit_aghera Rajkot
Sounds good then.
Keeping the TranslatableMarkup return type as it is and adding void return type in test method. - Status changed to Needs work
over 1 year ago 3:15pm 26 April 2023 - 🇺🇸United States smustgrave
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.
- 🇮🇳India mohit_aghera Rajkot
- Added a change record here https://www.drupal.org/node/3356831 →
- Fixed one more issue spotted related to usage of DI
- Updated the patch. - Status changed to Needs review
over 1 year ago 9:30am 27 April 2023 - last update
over 1 year ago 29,360 pass - Status changed to Needs work
over 1 year ago 4:50pm 27 April 2023 - 🇺🇸United States smustgrave
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.
- Status changed to RTBC
over 1 year ago 4:57pm 27 April 2023 - last update
over 1 year ago 29,362 pass - 🇺🇸United States smustgrave
Actually it was so small I didn't mind making the change.
- last update
over 1 year ago 29,367 pass - last update
over 1 year ago 29,368 pass - last update
over 1 year ago 29,375 pass - last update
over 1 year ago 29,379 pass - last update
over 1 year ago 29,380 pass - Status changed to Needs review
over 1 year ago 10:09am 8 May 2023 - 🇫🇮Finland lauriii Finland
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.
- Status changed to Needs work
over 1 year ago 1:43pm 8 May 2023 - 🇺🇸United States smustgrave
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?
- 🇮🇳India sharma.amitt16 Delhi
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. - 🇮🇳India mohit_aghera Rajkot
- 🇵🇹Portugal adaragao
Please, re-roll patch against 10.3.x.
I wish I could do it my self.... some day...
Thank you in advance !