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

Merged
merged 17 commits into from
Nov 11, 2024
Merged

Make Mage_GiftMessage optional in templates #4266

merged 17 commits into from
Nov 11, 2024

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 force-pushed the withoutGiftMessage branch from c1a9aca to 92439f8 Compare October 15, 2024 07:51
@Hanmac Hanmac marked this pull request as ready for review October 15, 2024 08:05
@Hanmac Hanmac force-pushed the withoutGiftMessage branch from ae84c75 to d823857 Compare October 16, 2024 05:17
@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.

@Hanmac Hanmac force-pushed the withoutGiftMessage branch from d823857 to 26ad492 Compare October 16, 2024 11:04
@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

@Hanmac Hanmac force-pushed the withoutGiftMessage branch from 6a59fc4 to ebeed07 Compare October 30, 2024 11:23
@Hanmac Hanmac requested review from sreichel and kiatng October 30, 2024 13:55
@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()

@kiatng kiatng merged commit 2969973 into OpenMage:main Nov 11, 2024
18 checks passed
@Hanmac Hanmac deleted the withoutGiftMessage branch November 11, 2024 09:39
fballiano added a commit to MahoCommerce/maho that referenced this pull request Nov 11, 2024
@fballiano
OM PR 4266
d8c480e 
OpenMage/magento-lts#4266
@sreichel sreichel mentioned this pull request Nov 11, 2024
Merged
fballiano
fballiano reviewed Nov 11, 2024
View reviewed changes
app/design/frontend/rwd/default/template/bundle/email/order/items/shipment/default.phtml
Comment on lines -84 to +88
<?php echo $this->escapeHtml($_item->getDescription()) ?>
<?php echo $this->escapeHtml($parentItem->getDescription()) ?>
Copy link
Contributor

Choose a reason for hiding this comment

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

is this correct? changing from item to parentitem?

This was referenced Nov 12, 2024
Closed
Merged
sreichel added a commit to sreichel/magento-lts that referenced this pull request Nov 12, 2024
@sreichel
update
c53f177 
-see OpenMage#4266
sreichel added a commit to sreichel/magento-lts that referenced this pull request Nov 12, 2024
@sreichel
update
a249bf1 
-see OpenMage#4266
sreichel added a commit that referenced this pull request Nov 26, 2024
@sreichel @kiatng
Cache isModuleOutputEnabled or isModuleEnabled status (#4323)
ff2e140 
* cache module output status

* revert config caching

#4323 (comment)

* added scope label to config

* deprecated Mage_Adminhtml_Block_Template::isOutputEnabled()

- use Mage_Core_Block_Template::isModuleOutputEnabled()

* use new method

* added Mage_Core_Block_Template::isModuleEnabled()

* moved to abstract class

* added Mage_Core_Model_Resource_Abstract::isModuleEnabled()

* added Mage_Eav_Model_Entity_Collection_Abstract::isModuleEnabled()

* use new methods

* use trait

* updated baseline

* copyright [skip ci]

* Revert "use trait"

This reverts commit 51c7171

* update

-see #4266

---------

Co-authored-by: Ng Kiat Siong <kiatsiong.ng@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fballiano fballiano fballiano left review comments

@addison74 addison74 addison74 approved these changes

@sreichel sreichel sreichel approved these changes

@kiatng kiatng Awaiting requested review from kiatng

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
5 participants
@Hanmac @sreichel @fballiano @kiatng @addison74