Skip to content

Conversation

@sreichel
Copy link
Contributor

@sreichel sreichel commented Oct 15, 2025

This prevents calling methods on non existing blocks.

  • added some helper methods for common block (rout, head, breadcrumbs)
  • deprecated get Block() in favor of getBlockByName() that returns null or existing block, allowing to use PHP´8 null safe operator

@github-actions github-actions bot added Component: PayPal Relates to Mage_Paypal Component: Core Relates to Mage_Core Component: Catalog Relates to Mage_Catalog Component: Cms Relates to Mage_Cms Component: Checkout Relates to Mage_Checkout Component: Sales Relates to Mage_Sales Component: Customer Relates to Mage_Customer Component: Adminhtml Relates to Mage_Adminhtml Component: Page Relates to Mage_Page Component: Tag Relates to Mage_Tag Component: Wishlist Relates to Mage_Wishlist Component: Review Relates to Mage_Review Component: Newsletter Relates to Mage_Newsletter Component: Downloadable Relates to Mage_Downloadable Component: CatalogSearch Relates to Mage_CatalogSearch Component: Rss Relates to Mage_Rss labels Oct 15, 2025
@Hanmac
Copy link
Contributor

Hanmac commented Oct 16, 2025

I dislike that the new functions still returns false on missing block.
(that functions can return false instead of null is something I dislike about old magento core)
If they returned null instead, then you could use ?-> the Null-Safe Operator

    $this->getLayout()->getBlockAdminhtmlHead()?->setCanLoadTinyMce(true);

@sreichel sreichel marked this pull request as ready for review October 20, 2025 08:27
Copilot AI review requested due to automatic review settings October 20, 2025 08:27
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR introduces a unified approach for creating and accessing common layout blocks by adding type-safe helper methods to Mage_Core_Model_Layout. The changes replace direct getBlock() calls with new specialized methods like getBlockHead(), getBlockRoot(), and getBlockBreadcrumbs() that return properly typed blocks or null, leveraging PHP's nullsafe operator for cleaner code.

  • Adds new typed block accessor methods (getBlockHead(), getBlockRoot(), getBlockBreadcrumbs(), etc.) to Mage_Core_Model_Layout
  • Refactors all block access patterns across the codebase to use these new methods with nullsafe operator
  • Deprecates the legacy getBlock() method in favor of getBlockByName()

Reviewed Changes

Copilot reviewed 71 out of 71 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
app/code/core/Mage/Core/Model/Layout.php Adds new typed block accessor methods and deprecates getBlock()
app/code/core/Mage/Wishlist/Block/Share/Wishlist.php Refactors head block access using getBlockHead()
app/code/core/Mage/Wishlist/Block/Customer/Wishlist.php Refactors head block access using getBlockHead()
app/code/core/Mage/Wishlist/Block/Customer/Sharing.php Refactors head block access using getBlockHead()
app/code/core/Mage/Tag/controllers/CustomerController.php Refactors head block access using getBlockHead()
app/code/core/Mage/Tag/Block/Product/Result.php Refactors head and root block access using new methods
app/code/core/Mage/Sales/controllers/OrderController.php Refactors head block access using getBlockHead()
app/code/core/Mage/Sales/Helper/Guest.php Refactors breadcrumbs block access using getBlockBreadcrumbs()
app/code/core/Mage/Sales/Block/Order/*.php Refactors head block access across multiple order-related blocks
app/code/core/Mage/Rss/Block/List.php Refactors head block access with improved null handling
app/code/core/Mage/Review/controllers/*.php Refactors head and breadcrumbs block access
app/code/core/Mage/Paypal/Controller/Express/Abstract.php Refactors root block access with null check
app/code/core/Mage/Page/Helper/Layout.php Refactors root block access using getBlockRoot()
app/code/core/Mage/Newsletter/controllers/ManageController.php Refactors head block access using getBlockHead()
app/code/core/Mage/Downloadable/controllers/CustomerController.php Refactors head block access using getBlockHead()
app/code/core/Mage/Customer/controllers/AccountController.php Refactors head block access using getBlockHead()
app/code/core/Mage/Customer/Block/*.php Refactors head block access across customer blocks
app/code/core/Mage/Core/Controller/Varien/Action.php Refactors head block access in title rendering
app/code/core/Mage/Cms/Block/Page.php Refactors breadcrumbs, root, and head block access
app/code/core/Mage/Checkout/controllers/*.php Refactors head and root block access across checkout
app/code/core/Mage/Checkout/Block/Multishipping/*.php Refactors head block access across multishipping blocks
app/code/core/Mage/CatalogSearch/Block/*.php Refactors breadcrumbs and head block access
app/code/core/Mage/Catalog/controllers/CategoryController.php Refactors root block access with improved method chaining
app/code/core/Mage/Catalog/Block/*.php Refactors head and breadcrumbs block access across catalog
app/code/core/Mage/Adminhtml/controllers/*.php Refactors adminhtml head block access using getBlockAdminhtmlHead()
app/code/core/Mage/Adminhtml/Controller/Action.php Refactors menu and breadcrumbs block access with type declarations
app/code/core/Mage/Adminhtml/Block/*.php Refactors adminhtml head and breadcrumbs block access
app/code/core/Mage/Adminhtml/Block/Page/Menu.php Adds method docblocks for magic methods
app/code/core/Mage/Adminhtml/Block/Page/Head.php Adds method docblocks for magic methods
app/code/core/Mage/Adminhtml/Block/System/Email/Template/Edit/Form.php Refactors head block access and method chaining
.rector.php Adds commented-out rector rule for future method renaming
Comments suppressed due to low confidence (1)

app/code/core/Mage/Adminhtml/Block/Page/Head.php:1

  • Missing docblock for setCanLoadTinyMce() method which is used throughout the codebase (e.g., in Newsletter/Template/Edit.php, Cms/Block/Edit/Form.php). Add: @method $this setCanLoadTinyMce(bool $value)
<?php

@Hanmac
Copy link
Contributor

Hanmac commented Oct 20, 2025

I'm with copilot there: Breaking the method chain reduces readability

@sreichel
Copy link
Contributor Author

Okay, change it.

@sreichel sreichel added this to the 20.16.0 milestone Oct 22, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

Copilot reviewed 71 out of 71 changed files in this pull request and generated 11 comments.

@Hanmac
Copy link
Contributor

Hanmac commented Oct 22, 2025

i don't know why getDefaultTitle is needed when both title_prefix and title_suffix exist
🤷‍♂️
but that might be a different issue

@sreichel
Copy link
Contributor Author

You are right. "Reference in new issue" and move on?

@sonarqubecloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
5.6% Duplication on New Code (required ≤ 3%)

See analysis details on SonarQube Cloud

@sreichel sreichel modified the milestones: 20.16.0, 20.17.0 Oct 31, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Component: CatalogSearch Relates to Mage_CatalogSearch Component: Checkout Relates to Mage_Checkout Component: Cms Relates to Mage_Cms Component: Core Relates to Mage_Core Component: Customer Relates to Mage_Customer Component: Downloadable Relates to Mage_Downloadable Component: Newsletter Relates to Mage_Newsletter Component: Page Relates to Mage_Page Component: PayPal Relates to Mage_Paypal Component: Review Relates to Mage_Review Component: Rss Relates to Mage_Rss Component: Sales Relates to Mage_Sales Component: Tag Relates to Mage_Tag Component: Wishlist Relates to Mage_Wishlist rector

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants