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

Already on GitHub? Sign in to your account

Remove "remove" tags from checkout #81

Closed
barbazul opened this Issue Sep 14, 2012 · 4 comments

Comments

3 participants
Contributor

barbazul commented Sep 14, 2012

There are a bunch of unnecessary <remove> tags in checkout.xml (and now in the corresponding layout.xml file) which remove either the left or the right column.

By unnecessary I mean that they are usually redundant with the template being assigned to root. i.e. in <checkout_cart_index> root template is page/1column.phtml but there are also a <remove name="left" /> and <remove name="right" /> tags. Same thing happens for almost every other handler in that layout.

This is a problem because there is no <unremove> (I'm fine with that btw) so there is no way to change the root layout of checkout pages via local.xml

Contributor

magento-team commented Oct 9, 2012

@barbazul
Thank you for pointing this out. We have cleaned up all similar places where "remove" directive was excessive or referred to unused elements (will be rolled out in next updates).

In some places the "remove" directive cannot be deleted without substantial refactoring -- that's when it is used for purpose other from design customization. In some other rare places it is still used for design customization, but it is only in specific themes.

We will consider adopting a convention to refrain using "remove" directive in Magento Core code, so that 3rd-party customizations will have better flexibility.

Contributor

benmarks commented Oct 25, 2012

Apologies if I've missed related discussion somewhere, but if not:

It should be noted that the <remove /> directive is not functionally void in the context of <checkout_cart_index />. It saves the overhead of instantiating several child blocks which won't appear if the right block is not rendered, which is the case with the checkout/cart/index view by default. It would be appropriate for the core team to consider this.

That said, this topic comes up in literally every Magento U Fundamentals class. I characterize it as follows: "<remove /> is the 'unringable bell' where layout update XML is concerned; because of this, it should not be used by the core", which would leave available the option of using these blocks or their children. In other words, it should be left up to the end implementer to explicitly remove blocks which will not be rendered.

Contributor

magento-team commented Oct 25, 2012

@benmarks

It saves the overhead of instantiating several child blocks which won't appear if the right block is not rendered, which is the case with the checkout/cart/index view by default. It would be appropriate for the core team to consider this.

In current implementation of layout model (in Magento 2.x), there will be no blocks instantiated until all instructions of layout are read. We decoupled the process of building bare structure from creating full-fledged elements. In other words, we implement "lazy initialization" pattern for instantiating blocks when generating layout. You can inspect the Mage_Core_Model_Layout::generateElements() to see how exactly it is done.

Contributor

barbazul commented Oct 25, 2012

@mage2-team That's good news!

magento-team added a commit that referenced this issue Nov 12, 2012

2.0.0.0-dev30
* Framework changes
  * Added dependency injection of framework capability
    * Adopted Zend\Di component of Zend Framework 2 library
    * Implemented object manager in Magento application
    * Refactored multiple base classes to dependency injection principle (dependencies are declared in constructor)
  * Themes/View
    * Implemented storing themes registry in database, basic CRUD of themes, automatic registration of themes in database from file system out of the box
    * Renamed `Mage_Core_Model_Layout_Update` into `Mage_Core_Model_Layout_Merge`, the former becomes an entity domain model. Similar changes with `Mage_Core_Model_Resource_Layout` -> `Mage_Core_Model_Resource_Layout_Update`, `Mage_Core_Model_Layout_Data` -> `Mage_Core_Model_Layout_Update`
* Performance tests
  * Improved indexers running script `dev/shell/indexer.php` to return appropriate exit code upon success/failure
  * Implemented running the same performance scenario file with different parameters
  * Slightly refactored framework class `Magento_Performance_Testsuite_Optimizer` for better visibility of algorithm
* Visual design editor
  * Added ability to remove elements in editor UI
  * Revised history of changes VDE toolbar and algorithm of "compacting" operations (moving, removing elements) as a layout update XML
  * Added selection of themes to VDE launcher page
* Refactored JavaScript of some UI elements to jQuery:
  * "Simple" and "configurable" product view pages
  * "Create Account" page
  * "Shopping Cart" page
  * CAPTCHA
  * Newsletter subscription
* Tax management UX improvements
  * Split Basic and Advanced Settings for Tax Rule Management UI
  * Moved the Import/Export functionality to Tax Rate page
  * Moved Tax menu to System from Sales
* Implemented the editable multiselect JavaScript component
* Added mentioning sitemap in `robots.txt` after generation
* Removed creation of DB backup in integration testing framework
* Fixed logic of order of loading ACL resources in backend
* Fixed JavaScript error during installation when one of files in `pub/media` is not writable
* Fixed structure of legacy test fixtures that allowed ambiguous keys in declaration
* Fixed inability to restore admin password when CAPTCHA is enabled
* Various minor UX fixes (labels, buttons, redirects, etc...)
* GitHub requests:
  * [#59](#59) -- implemented handling of unexpected situations in admin/dashboard/tunnel action
  * [#66](#66)
    * refactored ImageMagick adapter unit test to avoid system operation
    * simplified unit testing framework -- removed unused classes, simplified handling logic of directory `dev/tests/unit/tmp` and removed it from VCS
  * [#73](#73), [#74](#74) -- fixes in docblock tags
  * [#75](#75), [#96](#96) -- fixed translation module contexts in a few places
  * [#80](#80) -- fixed some runtime errors in import/export module
  * [#81](#81) -- removed usage of "remove" directive in places where it is overridden by setting root template anyway
  * [#87](#87) -- changed paths of files to include from relative into absolute in `dev/shell/indexer.php` and `log.php`
  * [#88](#88) -- provided comments for values that can be configured in `app/etc/local.xml` file
  * [#90](#90) -- slightly optimized logic of implementation of loading configurable product attributes

vpelipenko added a commit that referenced this issue Feb 20, 2015

Merge pull request #81 from magento-mpi/MAGETWO-33035
[MPI] Multi-tenant mode and DI configuration optimization

MomotenkoNatalia added a commit that referenced this issue Oct 31, 2015

magento-team pushed a commit that referenced this issue Mar 23, 2016

Merge pull request #81 from AnastasiaBuniakEPAM/JS-323
JS-323: Gallery setting for configurable product

-Static test fix

okorshenko added a commit that referenced this issue Jun 7, 2016

@magento-engcom-team magento-engcom-team moved this from TODO to Done in branch [2.3-develop] Sep 12, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment