Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Page layouts are hard-coded in Magento\Widget\Block\Adminhtml\Widget\Instance\Edit\Chooser\Container #30471

Closed
1 of 5 tasks
ericmorand opened this issue Oct 12, 2020 · 11 comments · Fixed by #31886
Closed
1 of 5 tasks
Assignees
Labels
Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: ready for confirmation Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: done Reported on 2.3.5 Indicates original Magento version for the Issue report. Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it

Comments

@ericmorand
Copy link

ericmorand commented Oct 12, 2020

Preconditions (*)

Magento 2.3.5

Steps to reproduce (*)

Have a look at the code source of Magento\Widget\Block\Adminhtml\Widget\Instance\Edit\Chooser\Container.

Expected result (*)

The function getPageLayouts() should return the actual list of page layouts declared by the different modules.

Actual result (*)

The function getPageLayouts() only returns the one provided by Magento_Theme, and they are hard-coded in the file:

    /**#@+
     * Frontend page layouts
     */
    const PAGE_LAYOUT_1COLUMN = '1column-center';
    const PAGE_LAYOUT_2COLUMNS_LEFT = '2columns-left';
    const PAGE_LAYOUT_2COLUMNS_RIGHT = '2columns-right';
    const PAGE_LAYOUT_3COLUMNS = '3columns';
    /**#@-*/

   [...]

  /**
     * Retrieve page layouts
     *
     * @return array
     */
    protected function getPageLayouts()
    {
        return [
            self::PAGE_LAYOUT_1COLUMN,
            self::PAGE_LAYOUT_2COLUMNS_LEFT,
            self::PAGE_LAYOUT_2COLUMNS_RIGHT,
            self::PAGE_LAYOUT_3COLUMNS,
        ];
    }

Workaround

I may be possible to override Magento\Widget\Block\Adminhtml\Widget\Instance\Edit\Chooser\Container and to inject an instance of Magento\Framework\View\Model\PageLayout\Config\BuilderInterface to grab the list of available page layouts from there. Untested though.

Thoughts

Basically, it makes any container with Label declared in a non-Magento Theme page layout not selectable from the back-office to inject widgets. But the simple fact that Magento_Widget assumes that some page layouts exist is concerning. This concern is actually confirmed by the fact that one of the four hard-coded page layouts doesn't even exist: 1column-center.


Please provide Severity assessment for the Issue as Reporter. This information will help during Confirmation and Issue triage processes.

  • Severity: S0 - Affects critical data or functionality and leaves users without workaround.
  • Severity: S1 - Affects critical data or functionality and forces users to employ a workaround.
  • Severity: S2 - Affects non-critical data or functionality and forces users to employ a workaround.
  • Severity: S3 - Affects non-critical data or functionality and does not force users to employ a workaround.
  • Severity: S4 - Affects aesthetics, professional look and feel, “quality” or “usability”.
@m2-assistant
Copy link

m2-assistant bot commented Oct 12, 2020

Hi @ericmorand. Thank you for your report.
To help us process this issue please make sure that you provided the following information:

  • Summary of the issue
  • Information on your environment
  • Steps to reproduce
  • Expected and actual results

Please make sure that the issue is reproducible on the vanilla Magento instance following Steps to reproduce. To deploy vanilla Magento instance on our environment, please, add a comment to the issue:

@magento give me 2.4-develop instance - upcoming 2.4.x release

For more details, please, review the Magento Contributor Assistant documentation.

Please, add a comment to assign the issue: @magento I am working on this


⚠️ According to the Magento Contribution requirements, all issues must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of issues happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

@ericmorand
Copy link
Author

I can now confirm that overriding the class works perfectly:

<?php


namespace [Vendor]\[Theme]\Block\Adminhtml\Widget\Instance\Edit\Chooser;


class Container extends \Magento\Widget\Block\Adminhtml\Widget\Instance\Edit\Chooser\Container
{
    /** @var \Magento\Framework\View\Model\PageLayout\Config\BuilderInterface */
    protected $_pageLayoutConfigBuilder;

    public function __construct(
        \Magento\Framework\View\Element\Context $context,
        \Magento\Framework\View\Layout\ProcessorFactory $layoutProcessorFactory,
        \Magento\Theme\Model\ResourceModel\Theme\CollectionFactory $themesFactory,
        \Magento\Framework\View\Model\PageLayout\Config\BuilderInterface $pageLayoutConfigBuilder,
        array $data = []
    ) {
        $this->_pageLayoutConfigBuilder = $pageLayoutConfigBuilder;

        parent::__construct($context, $layoutProcessorFactory, $themesFactory, $data);
    }

    protected function getPageLayouts()
    {
        $pageLayoutsConfig = $this->_pageLayoutConfigBuilder->getPageLayoutsConfig();

        return array_keys($pageLayoutsConfig->getPageLayouts());
    }
}

This Container returns the actual list of page layouts instead of the hard-coded and wrong one of the core implementation.

@sdzhepa sdzhepa added the Reported on 2.3.5 Indicates original Magento version for the Issue report. label Nov 11, 2020
@stale
Copy link

stale bot commented Jan 26, 2021

This issue has been automatically marked as stale because it has not had recent activity. It will be closed after 14 days if no further activity occurs. Is this issue still relevant? If so, what is blocking it? Is there anything you can do to help move it forward? Thank you for your contributions!

@stale stale bot added the stale issue label Jan 26, 2021
@mrtuvn
Copy link
Contributor

mrtuvn commented Jan 27, 2021

This issue still quite important at my point of view
CC: @ihor-sviziev @sidolov

@stale stale bot removed the stale issue label Jan 27, 2021
@engcom-Alfa engcom-Alfa added the Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it label Jan 27, 2021
@ihor-sviziev
Copy link
Contributor

@ericmorand @mrtuvn, could you explain the use cases when these values need to be changed/customized?

@mrtuvn
Copy link
Contributor

mrtuvn commented Jan 27, 2021

Should getPageLayouts get all layout instead return fixed value array ? Example i create custom page layout somewhere else in theme Should magento update while widget choose page layout from back office ? Maybe i missed something or magento update value from other place

@ihor-sviziev ihor-sviziev added the Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. label Jan 27, 2021
@ihor-sviziev
Copy link
Contributor

@mrtuvn yeah. Make sense! I think it should be marked with S3 P2.
@sidolov @sivaschenko @gabrieldagama, what do you think?

Hi @ericmorand,
Could you create a pull request with suggested changes from the following comment #30471 (comment) ?

@sivaschenko
Copy link
Member

Added to Public Triage Meeting agenda on Thursday.

@gabrieldagama gabrieldagama added the Priority: P2 A defect with this priority could have functionality issues which are not to expectations. label Jan 28, 2021
@gabrieldagama gabrieldagama added Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed labels Jan 28, 2021
@magento-engcom-team
Copy link
Contributor

✅ Confirmed by @gabrieldagama
Thank you for verifying the issue. Based on the provided information internal tickets MC-40669 were created

Issue Available: @gabrieldagama, You will be automatically unassigned. Contributors/Maintainers can claim this issue to continue. To reclaim and continue work, reassign the ticket to yourself.

magento-engcom-team added a commit that referenced this issue Mar 7, 2021
…et\Block\Adminhtml\Widget\Instance\Edit\Chooser\Container #31886

 - Merge Pull Request #31886 from Bartlomiejsz/magento2:bugfix/30471_page_layouts_hardcoded
 - Merged commits:
   1. 1b9f190
   2. 2ea06fb
   3. db81b6b
   4. 23db47e
magento-engcom-team pushed a commit that referenced this issue Mar 7, 2021
…et\Block\Adminhtml\Widget\Instance\Edit\Chooser\Container #31886
@ericmorand
Copy link
Author

ericmorand commented Mar 8, 2021

@ihor-sviziev , thanks a lot for taking care of this. Much appreciated.

To give a little bit of context on this issue and most of the other issues I've opened related to layouts, widgets and themes, we had to work on a custom design for a customer, based on the job from a creative agency. We didn't want to base our work on an existing Magento 2 theme (too much to undo) but still wanted to propose to our client to benefit fully from the themes, widgets, layout and per-product/per-category customization of Magento 2.

We then went to full custom way, providing two themes, custom page layouts and an extensive set of widgets. But equally important, we wanted to provide our customer with a taylor-made back-office where only what was supported by our themes was available to the webmasters. So, we wanted to have only the page layouts supported by our themes visible in the back-office, for example.

We had to cusotmize a lot of things and implement some "separation of concerns" strategy that were not provided by Magento 2 (at least from the documentation) like preventing modules from providing page layouts (letting the themes being fully responsible about that) or having a separate project based on Twig / ES6 / SASS for our frontend developers, writing specification for every templates used by our Blocks, forbiding sending anything else than primitives variables to the templates (no classes, no functions, no helpers), and so on.

The final result was above our expectations in product quality, project quality (the maintainablity of the thing is leagues above everything we did with Magento 2 in the past), budget and delay.

My own personal conclusion about Magento 2 after that experience is that the framework is amazing (it allowed us to implement everything that we wanted easily even though our approach was quite disruptive, which says a lot of the quality of a framework) but that the documentation and the "recommended way of doing things" is quite dated. About that last point, we had to fight a lot against legacy Magento 2 developers and architects that didn't believe in our approach and threw the "not a good practice" argument to our faces. :D

It was worth it:

https://www.girard-perregaux.com/

Plus we now have an extensive understanding of the whole page rendering process of Magento 2, end-to-end. :D

@mrtuvn
Copy link
Contributor

mrtuvn commented Mar 8, 2021

Thanks for your sharing such care
in my opinion magento 2 is great framework. Now you can do a lot with current tools. headless approach with magento only as backend and much more. No limited. But magento also keep bigger and bigger. This big train hard to turn immediately. Now it's already Implements a lot new business logic. But I think adobe should more listen on developers experience instead focus on they business. Avoid break changes to lowest as possible. I still believe current community can keep strong and expand to more

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Issue: Confirmed Gate 3 Passed. Manual verification of the issue completed. Issue is confirmed Issue: ready for confirmation Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: done Reported on 2.3.5 Indicates original Magento version for the Issue report. Reproduced on 2.4.x The issue has been reproduced on latest 2.4-develop branch Severity: S3 Affects non-critical data or functionality and does not force users to employ a workaround. Triage: Dev.Experience Issue related to Developer Experience and needs help with Triage to Confirm or Reject it
Projects
Archived in project
10 participants