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

Can't cancel removal of a block or container in layout by setting remove attribute value to false #1931

Closed
thomasnordkvist opened this issue Sep 23, 2015 · 25 comments
Assignees
Labels
Area: Frontend bug report Fixed in 2.1.x The issue has been fixed in 2.1 release line Fixed in 2.2.x The issue has been fixed in 2.2 release line Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release

Comments

@thomasnordkvist
Copy link

I inherit the blank theme and i wanted to have the top.links visible in the checkout so i created this file:

Vendor/Theme/Magento_Checkout/layout/checkout_index_index.xml
containing this:

<?xml version="1.0"?>
<!--
/**
 * Copyright © 2015 Magento. All rights reserved.
 * See COPYING.txt for license details.
 */
-->
<page xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" layout="2columns-left" xsi:noNamespaceSchemaLocation="../../../../../../../lib/internal/Magento/Framework/View/Layout/etc/page_configuration.xsd">
    <body>
        <referenceContainer name="header.panel" remove="false"/>
    </body>
</page>

Reading This i understand, that it sould now be possible to cancel a removal instruction with the above instruction, but it doesn't seem to work.

I know the file is parsed, and included in the layout, because if i add a block from this file it will show up on the checkout page.

/Thomas

@okorshenko okorshenko added the PS label Sep 24, 2015
@maksek maksek added the forum label Sep 30, 2015
@daim2k5
Copy link
Contributor

daim2k5 commented Nov 10, 2015

The GitHub issue tracker is intended for technical issues only. Please refer to the Community Forums or Magento Stack Exchange site for technical questions. In your case, the programming questions forum is likely the most appropriate. Feel free to reopen this issue if you think you have encountered a bug in Magento 2.

@daim2k5 daim2k5 closed this as completed Nov 10, 2015
@philippsander
Copy link
Member

this sounds like a technical issue for me... I have the same problem.

EDIT: the documentation clearly says the the removeal process is canceled. that does not work!

@thomasnordkvist
Copy link
Author

I had almost forgotten this issue.
It should be reopened as a bug.

I did some copy paste to build up the parts i wanted back in the header.

@daim2k5 daim2k5 reopened this Apr 15, 2016
@daim2k5 daim2k5 removed the forum label Apr 15, 2016
@AntonEvers
Copy link
Contributor

This problem can be reproduced by removing an element in a parent theme and trying to cancel it in a child theme.

@BenSpace48
Copy link
Contributor

BenSpace48 commented Jun 1, 2016

Any news on this yet Magento? It doesn't even look it's been been internally raised or acknowledged yet.

CC @daim2k5

@dexterchua19
Copy link

Any news about this? I'm developing a website and this same problem occurs in me.

@pboisvert
Copy link

@tkacheva can you ask the team to investigate this one?

@tkacheva
Copy link

tkacheva commented Jul 1, 2016

@eug123 please take a look MAGETWO-54935

@philippsander
Copy link
Member

come on guys. almost 4 months to fix this?

@tkacheva
Copy link

tkacheva commented Jul 8, 2016

@philippsander issue is waiting to be merged

@Styopchik
Copy link

Still waiting

@vkorotun vkorotun added Area: Frontend and removed PS labels Aug 3, 2016
@tkacheva tkacheva added the Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development label Oct 6, 2016
mmansoor-magento pushed a commit that referenced this issue Oct 19, 2016
mmansoor-magento pushed a commit that referenced this issue Oct 19, 2016
Fixed issues:
- MAGETWO-57432: Functional test Magento\Bundle\Test\TestCase\CreateBundleProductEntityTest is failed
- MAGETWO-55510: "Client side less compilation" does not work properly #3325
- MAGETWO-54935: [Github #1931] Cancellation of element removal in layouts doesn't work
@torreytsui
Copy link
Contributor

torreytsui commented Nov 24, 2016

A limited workaround for the time being

If the purpose is simply hiding and displaying specific blocks and if you have full control on those layouts, you may interest in this limited workaround.

<!-- parent_layout_handle.xml -->
+ <referenceContainer name="root" display="false"/>
- <referenceContainer name="root" remove="true"/>

<!-- child_layout_handle.xml -->
+ <referenceContainer name="root" display="true"/>
- <referenceContainer name="root" remove="false"/>

Use display instead of remove. This workaround is very limited as we don't usually have full controls on all layout. For example, many Magento vanilla handles have remove="true" instructions and we basically cannot add those blocks back before a fix is released.

remove="false" seems not implemented

If you do need the remove="false" to work before a fix is released, there is a way of hacking.

// \Magento\Framework\View\Layout\Reader\Block::scheduleReference
protected function scheduleReference(
        Layout\ScheduledStructure $scheduledStructure,
        Layout\Element $currentElement
    ) {
        $elementName = $currentElement->getAttribute('name');
        $elementRemove = filter_var($currentElement->getAttribute('remove'), FILTER_VALIDATE_BOOLEAN);
        if ($elementRemove) {
            $scheduledStructure->setElementToRemoveList($elementName);
        } else {
            $data = $scheduledStructure->getStructureElementData($elementName, []);
            $data['attributes'] = $this->mergeBlockAttributes($data, $currentElement);
            $this->updateScheduledData($currentElement, $data);
            $this->evaluateArguments($currentElement, $data);
            $scheduledStructure->setStructureElementData($elementName, $data);
        }
    }

I suppose $scheduledStructre->unsetElementFromListToRemove($elementName) should be called in the else condition or something relevant so that the remove flag of the block will be reset and the block can be kept.

        if ($elementRemove) {
            $scheduledStructure->setElementToRemoveList($elementName);
        } else {
            $data = $scheduledStructure->getStructureElementData($elementName, []);
            $data['attributes'] = $this->mergeBlockAttributes($data, $currentElement);
            $this->updateScheduledData($currentElement, $data);
            $this->evaluateArguments($currentElement, $data);
            $scheduledStructure->setStructureElementData($elementName, $data);
+            $scheduledStructre->unsetElementFromListToRemove($elementName)
        }

Since there seems to be a fix waiting to merge, I'm not going to raise a PR.

@KrystynaKabannyk
Copy link

hi @thomasnordkvist, the issue has been fixed under MAGETWO-54935 in develop branch, you can find it here and apply for your version: 9bd170d

@PieterCappelle
Copy link
Contributor

Please merge this in some upcoming release. Thanks.

@korostii
Copy link
Contributor

korostii commented Jun 6, 2017

Hi @KrystynaKabannyk, could you please reopen this issue (until the fix appears in a released 2.1.x version)?
Thanks in advance.

@KrystynaKabannyk
Copy link

hi @korostii, this issue hasn't been scheduled to be backported, it will go to 2.2.0 release.

@korostii
Copy link
Contributor

korostii commented Jun 6, 2017

@KrystynaKabannyk
I hate to underline the obvious, but if the bug was discovered on 2.1.x and then reportedly "fixed", it is commonly expected to see that fix in the closest patch (2.1.x) release.

Considering that for many of the merchants the switch to 2.2 will probably be somewhat painful, resource-heavy (see this marvelous list of backwards-incompatible changes for a taste) and delayed for at least a couple more months, it would make sense to have it on the 2.1.x branch as well. Furthermore, someone from the community just might want this fix badly enough and make pull request aiming 2.1-develop.

Either that, or some disclaimer saying that "this is a known issue that won't ever be fixed in 2.1. Please move along to 2.2". You do know that there are people re-testing every bug they have encountered earlier after each new release in order to see if it got fixed? right?

Anyhow, having the issue marked as "closed" (while there isn't any straightforward way to use it available to ordinary users) is very misleading. Could you at least leave the issue open until 2.2.0 gets released?

@KrystynaKabannyk
Copy link

hi @korostii, we reviewed the issue again and made the decision to backport it into 2.1.x. Internal ticket for it is created MAGETWO-69715. Thank you for raising this issue again!

@PieterCappelle
Copy link
Contributor

Thank god for people like @korostii. Thanks for the effort @KrystynaKabannyk.

@magento-engcom-team magento-engcom-team added Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development bug report Area: Frontend Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed labels Sep 11, 2017
@magento-engcom-team
Copy link
Contributor

The issue already fixed and will be available in release 2.2.0 very soon

@magento-engcom-team magento-engcom-team added Fixed in 2.2.x The issue has been fixed in 2.2 release line Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release labels Sep 12, 2017
@korostii
Copy link
Contributor

korostii commented Sep 13, 2017

Hi @magento-engcom-team,
I am having a bad case of deja vu and I just hope that this message above was an automated one and someone has just missed the discussion above (as opposed to outright ignoring it and turning this thing over again).

The issue is reported and reproduced on 2.1.x
The issue isn't fixed on 2.1.x
How come it's "closed" already (again) ?!

Just trying to make sense of it, please help me understand the issue flow here.
I mean, this isn't your internal JIRA where the only thing that matters is whether is fix is implemented already. People here care whether they can update to the latest release in order to have the bug gone (and they can't, in this case)

@Morgon
Copy link

Morgon commented Nov 10, 2017

@magento-engcom-team @KrystynaKabannyk - Magento committed to backporting this into 2.1 and has marked the ticket as Done (even after @korostii mentioned that Magento may be misunderstanding). However, it's not even in 2.1.10-dev. Please fix the status of this report and update the community on an ETA.

@quisse
Copy link

quisse commented Mar 20, 2018

Still not backported to 2.1.*?

quisse pushed a commit to quisse/magento2 that referenced this issue Mar 23, 2018
…in layouts doesn't work

-- integration test
@magento-engcom-team magento-engcom-team added the Fixed in 2.1.x The issue has been fixed in 2.1 release line label Mar 26, 2018
@magento-engcom-team
Copy link
Contributor

Hi @thomasnordkvist. Thank you for your report.
The issue has been fixed in #14198 by @quisse in 2.1-develop branch
Related commit(s):

The fix will be available with the upcoming 2.1.14 release.

@quisse
Copy link

quisse commented Mar 27, 2018

@magento-engcom-team Do you mean 2.1.13 or is that one going to be skipped?

@quisse quisse self-assigned this Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Frontend bug report Fixed in 2.1.x The issue has been fixed in 2.1 release line Fixed in 2.2.x The issue has been fixed in 2.2 release line Issue: Format is not valid Gate 1 Failed. Automatic verification of issue format is failed Issue: Ready for Work Gate 4. Acknowledged. Issue is added to backlog and ready for development Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release
Projects
None yet
Development

No branches or pull requests