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

Removed "admin routing compatibility mode" for extensions #1551

Merged
merged 5 commits into from
Sep 19, 2023
Merged

Removed "admin routing compatibility mode" for extensions #1551

merged 5 commits into from
Sep 19, 2023

Conversation

luigifab
Copy link
Contributor

@luigifab luigifab commented Apr 16, 2021

Description

After #1470, this PR remove Admin routing compatibility mode for extensions.
Yes, it can break some old modules 💥 .

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 Component: Core Relates to Mage_Core translations Relates to app/locale labels Apr 16, 2021
@Flyingmana Flyingmana changed the title Removed back doors Remove Admin routing compatibility mode for extensions Apr 17, 2021
@fballiano
Copy link
Contributor

would it be possible to also delete the admin/security/extensions_compatibility_mode entry from core_config_data?

@Flyingmana
Copy link
Contributor

is there a specific reason for this @fballiano? I ask, because this would break the ability to roll back to the previous version, while I not see a great advantage in return.

@fballiano
Copy link
Contributor

is there a specific reason for this @fballiano? I ask, because this would break the ability to roll back to the previous version, while I not see a great advantage in return.

mmm I thought about not leaving an orphan record that will never be used again but yes, the rollback possibility is probably worth leaving it there :-)

@luigifab luigifab marked this pull request as ready for review April 30, 2021 09:52
@luigifab luigifab changed the title Remove Admin routing compatibility mode for extensions (BC) Remove Admin routing compatibility mode for extensions May 16, 2021
@sreichel sreichel added the backwards compatibility Might affect backwards compatibility for some users label Jan 3, 2023
Copy link
Contributor

@sreichel sreichel left a comment

Choose a reason for hiding this comment

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

I'd not remove that code, but set default value to 0 in config.xml (and update README with link to information what this is about).

... and change branch to v19

@fballiano fballiano changed the base branch from 20.0 to main April 4, 2023 17:31
@fballiano fballiano changed the base branch from main to next July 4, 2023 14:51
Copy link
Contributor

@fballiano fballiano left a comment

Choose a reason for hiding this comment

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

I think this one should be merged as-is in next

@fballiano fballiano changed the title (BC) Remove Admin routing compatibility mode for extensions Removed Admin routing compatibility mode for extensions Jul 15, 2023
@elidrissidev
Copy link
Member

I'm not very familiar with the old way of defining admin routing, but I did some searching within the codebase and it seems that Mage_Adminhtml is still using the old approach, not sure if that should be changed as part of this PR?

<routers>
<adminhtml>
<use>admin</use>
<args>
<module>Mage_Adminhtml</module>
<frontName>admin</frontName>
</args>
</adminhtml>
</routers>

@kyrena
Copy link
Contributor

kyrena commented Aug 4, 2023

I think that for Mage_Adminhtml it's normal and not a problem.
But it might be a good idea to add an example somewhere: bad and old way: ..., good way: ....

@fballiano
Copy link
Contributor

I've re-tested this PR today and all of the backend works fine, @elidrissidev do you still have doubts about it?

Copy link
Contributor

@kiatng kiatng left a comment

Choose a reason for hiding this comment

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

+1 for security.

@fballiano
Copy link
Contributor

great, I'll wait a bit more and then merge it, it's in next anyway so..

@elidrissidev
Copy link
Member

LGTM

@fballiano
Copy link
Contributor

oki then :-)

@addison74
Copy link
Contributor

This one needs a note in the README file.

@fballiano fballiano changed the title Removed Admin routing compatibility mode for extensions Removed "admin routing compatibility mode" for extensions Sep 19, 2023
@fballiano fballiano merged commit 14ad537 into OpenMage:next Sep 19, 2023
13 checks passed
@fballiano
Copy link
Contributor

@addison74 I'll add it manually in half an hour

@fballiano
Copy link
Contributor

@addison74 done in 5655b4b

fballiano added a commit that referenced this pull request Sep 19, 2023
@addison74
Copy link
Contributor

Very good, don't let anyone say that the modification was not made public ...

@luigifab luigifab deleted the remove-routing-compat branch September 20, 2023 17:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backwards compatibility Might affect backwards compatibility for some users Component: Core Relates to Mage_Core translations Relates to app/locale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants