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.

Sign up for GitHub

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

Jump to bottom

Make Mage_GiftMessage optional in templates #4266

Open
wants to merge 17 commits into
base: main
Choose a base branch
from
Open

Make Mage_GiftMessage optional in templates #4266

wants to merge 17 commits into from

Conversation

Hanmac
Copy link
Contributor

@Hanmac Hanmac commented Oct 10, 2024 •
edited
Loading

Description (*)

This makes all templates use functions in their blocks to check isModuleEnabled('Mage_GiftMessage') first

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Mage_GiftMessage shouldn't be required for Mage_Adminhtml #4263

Manual testing scenarios (*)

  1. ...
  2. ...

Questions or comments

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All automated tests passed successfully (all builds are green)
  • Add yourself to contributors list

@github-actions github-actions bot added Template : admin Relates to admin template Component: Checkout Relates to Mage_Checkout Component: Sales Relates to Mage_Sales Template : rwd Relates to rwd template Template : base Relates to base template Component: Adminhtml Relates to Mage_Adminhtml Component: Downloadable Relates to Mage_Downloadable Component: Bundle Relates to Mage_Bundle Component: Rss Relates to Mage_Rss labels Oct 10, 2024
@Hanmac Hanmac marked this pull request as ready for review October 15, 2024 08:05
@sreichel
Copy link
Contributor

Nice!

For the template you edit, can you please check that this is at top ...

/**
 * @see Mage_Some_Block_Name
 * @var Mage_Some_Block_Name $this
 */

@Hanmac
Copy link
Contributor Author

Hanmac commented Oct 16, 2024

Yeah, that annoyed me when i was looking at the templates

Does it need @see if it already has @var this?

@sreichel
Copy link
Contributor

No. var is more importan.

@sreichel
Copy link
Contributor

Great. :)

Can you please add return types to newly added methods?

@Hanmac
Copy link
Contributor Author

Hanmac commented Oct 18, 2024

canDisplayGiftmessage() could be existing methods, but I still touched them, so they should be updated?

@sreichel
Copy link
Contributor

To not break anything i'd not change existing methods.

@Hanmac
Copy link
Contributor Author

Hanmac commented Oct 18, 2024

To not break anything i'd not change existing methods.

imo, if getLayout->getBlock returns something else than Mage_Core_Block_Abstract,
their code is already broken, they just don't know it yet

@sreichel
Copy link
Contributor

Can you try to create an order with a downloadable product?

Uncaught Error: Class 'Mage_Giftmessage_Helper_Message'

@Hanmac
Copy link
Contributor Author

Hanmac commented Oct 28, 2024

Can you try to create an order with a downloadable product?

Uncaught Error: Class 'Mage_Giftmessage_Helper_Message'

I don't have a working shop right now, (vacation)

But can you give me a stacktrace to fix that?

@sreichel
Copy link
Contributor

sreichel commented Oct 29, 2024 •
edited
Loading 

Fatal error:  Uncaught Error: Class 'Mage_Giftmessage_Helper_Message' not found in /var/www/html/app/Mage.php:616
Stack trace:
#0 /var/www/html/app/code/core/Mage/Core/Model/Layout.php(627): Mage::helper('giftmessage/mes...')
#1 /var/www/html/app/code/core/Mage/Core/Block/Abstract.php(1105): Mage_Core_Model_Layout->helper('giftmessage/mes...')
#2 /var/www/html/app/code/core/Mage/Adminhtml/Block/Sales/Order/Create/Items/Grid.php(132): Mage_Core_Block_Abstract->helper('giftmessage/mes...')
#3 /var/www/html/app/design/adminhtml/default/default/template/sales/order/create/items/grid.phtml(427): Mage_Adminhtml_Block_Sales_Order_Create_Items_Grid->isGiftMessagesAvailable()
#4 /var/www/html/app/code/core/Mage/Core/Block/Template.php(272): include('/var/www/html/a...')
#5 /var/www/html/app/code/core/Mage/Core/Block/Template.php(309): Mage_Core_Block_Template->fetchView('adminhtml/defau...')
#6 /var/www/html/app/code/core/Mage/Core/Block/Template.php(322): Mage_Core_Block_Template->renderView()
#7 /var/www/html/app/code/core/Mage/A in /var/www/html/app/Mage.php on line 616

Better enjoy your vacation :)

@sreichel sreichel changed the title Make Mage_GiftMessage optional in templates Make Mage_GiftMessage optional in templates Oct 29, 2024
@sreichel
Copy link
Contributor

Very nice. Thanks for fixing phpstan issues too.

First i thought it would be small change ... but no. :)

Lets wait for #4323. It could replace all Mage::helper('core')->isModuleOutputEnabled()

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

@sreichel sreichel sreichel left review comments

@kiatng kiatng Awaiting requested review from kiatng

At least 2 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Bundle Relates to Mage_Bundle Component: Checkout Relates to Mage_Checkout Component: Downloadable Relates to Mage_Downloadable Component: Rss Relates to Mage_Rss Component: Sales Relates to Mage_Sales phpstan Template : admin Relates to admin template Template : base Relates to base template Template : rwd Relates to rwd template
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Mage_GiftMessage shouldn't be required for Mage_Adminhtml
3 participants
@Hanmac @sreichel @kiatng