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

Clean Adminhtml: Mage_Api blocks #4262

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

sreichel
Copy link
Contributor

@sreichel sreichel commented Oct 9, 2024

Description (*)

I'd like to get rid off Adminhtml. It is just a collection of classes that belong to a specific module. (in most cases)

My idea is to move all code to correct place and keep empty/deprecated adminhtml-classes for BC (like mysql4-classes).

To #2533 (comment)

I would definitely support this, but question:

if we are changing the block class names, are we updating these in all the layout xml files and in any PHP where we createBlock manually? Or do we continue to call adminhtml/blockname ?

All classes are still there and marked as deprecated. As soon i have finished it, there will be a module that holds all the deprecated classes.

No need for changes in 3rd-party-code!

Related Pull Requests

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Api PageRelates to Mage_Api PHPStorm labels Oct 9, 2024
@sreichel sreichel requested a review from kiatng October 14, 2024 04:55
# Conflicts:
#	.phpstan.dist.baseline.neon
@kiatng
Copy link
Contributor

kiatng commented Oct 15, 2024

How about moving the admihtml controllers to app\code\core\Mage\Api\controllers\Adminhtml?

@sreichel
Copy link
Contributor Author

Controllers, models later ... ?

controllers are not autoloaded, could be a problem when overriden in 3rd-party-code?

@kiatng
Copy link
Contributor

kiatng commented Oct 16, 2024

I think dismantling Mage_Adminhtml is breaking BC no matter what. For example, it will break override in app/code/local/Mage/Adminhtml.

@sreichel sreichel marked this pull request as draft October 18, 2024 11:42
@sreichel
Copy link
Contributor Author

#2533 (comment)

It should not break anything. The classes in "Mage/Adminhtml" are still there to override. Do i miss something?

# Conflicts:
#	.phpstan.dist.baseline.neon
#	app/code/core/Mage/Adminhtml/Block/Api/Role/Grid/User.php
#	app/code/core/Mage/Adminhtml/Block/Api/Tab/Rolesedit.php
#	app/code/core/Mage/Adminhtml/Block/Api/Tab/Userroles.php
#	app/code/core/Mage/Adminhtml/Block/Api/User/Grid.php
@colinmollenhour
Copy link
Member

I agree that it should have been organized this way all along, but man that is going to be a ton of work with lots of risk of regressions and no difference to the end user.. IF you're going to do it is there some automated way to do it? Also I'd suggest adding code to detect missed updates. E.g. in createBlock you can check for the old block aliases like adminhtml/api*.

@sreichel
Copy link
Contributor Author

Its "only" 15 modules, that have blocks in adminhtml.

Check for aliases is planned.

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: Api PageRelates to Mage_Api phpstan PHPStorm
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants