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

Fix Use Base Class Instead of Child Class when Referencing Constants #2904

Merged
merged 1 commit into from
Jan 10, 2023
Merged

Fix Use Base Class Instead of Child Class when Referencing Constants #2904

merged 1 commit into from
Jan 10, 2023

Conversation

Sdfendor
Copy link
Contributor

@Sdfendor Sdfendor commented Jan 6, 2023

Description (*)

There are two class constants being referenced by using a child class instead of using the base class, in which the constants are defined. Those constants aren't overriden. Therefore I don't think this usage of the child class is deliberate.
In itself this small mistake wouldn't bother me, but I encountered a scenario where those references cause problems. The constants aren't used withing Mage_Newsletter but e.g. in a block that is part of the email template grid in the admin area. In case one deactivates/removes Mage_Newsletter the code referencing Mage_Newsletter_Model_Template will fail.

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)

@github-actions github-actions bot added the Component: Adminhtml Relates to Mage_Adminhtml label Jan 6, 2023
@sreichel
Copy link
Contributor

sreichel commented Jan 6, 2023

I've deactivated Mage_Newsletter but dont see any error. Where does the error appear?

@fballiano
Copy link
Contributor

it you deactivate Mage_Newsletter it still continues to work, but it you really remove it from the file system than you'll get the error.

And since Mage_Adminhtml doesn't depend on Mage_Newsletter it's probably correct to merge this PR.

@Sdfendor
Copy link
Contributor Author

Sdfendor commented Jan 10, 2023

@sreichel @fballiano Yes I was wrong about the deactivation of the module. It solely happens when deleting the module. In our current OpenMage installation we strife to keep the footprint of the installation minimal so we delete modules entirely that we don't use. A place where the error happens with removed module Mage_Newsletter would be the transactional email grid: System > Transactional Emails.
That it doesn't happen for a deactivated module makes sense too, because the constant is referenced directly not indirectly by use of the appropriate factory method (Mage::getModel). Therefore the module deactivation has no effect, the autoloader doesn't consider whether a file is "deactivated" or not and the class is loaded regardless.

@fballiano fballiano merged commit 37fc6ed into OpenMage:1.9.4.x Jan 10, 2023
@fballiano
Copy link
Contributor

merged and cherry-picked to 20.0

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants