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

Fixed "if newsletter module disabled, customer backend would blank without notice" #3078

Merged
merged 1 commit into from
May 13, 2023

Conversation

sidealice
Copy link
Contributor

@sidealice sidealice commented Mar 10, 2023

removeable for the wishlist/giftmessage/sendtofriend/newsletter modules

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: lib/Varien Relates to lib/Varien Component: lib/* Relates to lib/* labels Mar 10, 2023
@addison74
Copy link
Contributor

This PR does not have a proper description. It would be natural to understand the issue, how to reproduce it and how to fix it. Looking in the source code and understanding the changes is not a thing at all a professional approach.

@kiatng
Copy link
Contributor

kiatng commented Mar 11, 2023

I cannot replicate this issue:

  1. I disable Mage_Newsletter in the Advanced tab in backend System Configuration.
  2. I disable the cache for Configurations, Layouts, and Blocks.
  3. I navigate to a backend customer page.
  4. I can see the tab Newsletter, on the page it's a grid. I do not see a blank page.

Question: did you remove the Newsletter code from the core?

@kiatng
Copy link
Contributor

kiatng commented Mar 11, 2023

A suggestion on PR: limit the file changes to one issue to make it easier to review. You have changes to files that do not seem to relate to Newsletter. Can you please remove those changes and commit again? You can make separate PRs for those which are removed.

@sidealice
Copy link
Contributor Author

I cannot replicate this issue:

  1. I disable Mage_Newsletter in the Advanced tab in backend System Configuration.
  2. I disable the cache for Configurations, Layouts, and Blocks.
  3. I navigate to a backend customer page.
  4. I can see the tab Newsletter, on the page it's a grid. I do not see a blank page.

Question: did you remove the Newsletter code from the core?

Yes, if disabled the module or remove the code you can see the problem.
And this would help to sepreate the modules out of core. In the future OM can maintain necessary core, the other modules e.g. newsletter/wishlist/giftmessage/sendtofriend could be a module better.

@sidealice
Copy link
Contributor Author

This PR does not have a proper description. It would be natural to understand the issue, how to reproduce it and how to fix it. Looking in the source code and understanding the changes is not a thing at all a professional approach.

Yes, I may update the description to ' support remove the wishlist/giftmessage/sendtofriend/newsletter module without issues'

@kiatng
Copy link
Contributor

kiatng commented Mar 12, 2023

As suggested by @elidrissidev, we need to do a proper separation of concerns. Mage_Customer depends on the following modules:

<depends>
<Mage_Dataflow/>
<Mage_Directory/>
<Mage_Eav/>
</depends>

All other modules here should be separated:

  1. Mage_Sales
  2. Mage_Catalog
  3. Mage_Newsletter
  4. Mage_Wishlist

Besides the tabs, which can be easily added with the event adminhtml_block_widget_tabs_html_before (see PR #1358), the rendering of the pages need to be separated as well:

image

Related to PR #2455. It means also PR #2533. And down the rabbit hole. What should we do?

@elidrissidev
Copy link
Member

After looking more into this, IMO it's better to keep the refactoring until another major version, since it means moving classes around which breaks BC. For now, handling with a condition is good enough.

@fballiano
Copy link
Contributor

I'll try to rebase in order to get this approved

@github-actions github-actions bot added phpstan PHPStorm phpunit and removed Component: lib/* Relates to lib/* Component: lib/Varien Relates to lib/Varien Component: Adminhtml Relates to Mage_Adminhtml labels May 12, 2023
@fballiano
Copy link
Contributor

this time it worked correctly, so I'm ok with this PR

@fballiano fballiano changed the title If newsletter module disabled, customer backend would blank without notice Fixed "if newsletter module disabled, customer backend would blank without notice" May 13, 2023
@fballiano fballiano merged commit 63595d0 into OpenMage:main May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants