-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
[4.0] [MVC] Always set model/view prefix, correct base path detection #25367
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
Conversation
|
All this pull request is doing is re-affirming my feeling that it is time to deprecate having separate extensions per application (minus templates). Because there are more hacks to "proxy" things than benefits this separation really brings. |
|
If this is about the messy code, it seems to be here for B/C-ish reasons. We could stop using |
Co-Authored-By: Brian Teeman <[email protected]>
Co-Authored-By: Brian Teeman <[email protected]>
|
I have tested this item ✅ successfully on cb8d6cc This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25367. |
1 similar comment
|
I have tested this item ✅ successfully on cb8d6cc This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25367. |
|
I have tested this item ✅ successfully on cb8d6cc This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25367. |
|
i suppose so |
|
RTC This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25367. |
|
@richard67, yes. Related issue #25344. |
|
@SharkyKZ Ha, I knew there was something like that. Other question. Are you still waiting for requested reviews, or are the good tests sufficient for you? |
|
It's up to maintainers now. Though I agree with @mbabker this is a rather ugly hack. We can probably do better here. |
|
Sure, but as far as I understand him, the right way would be far beyond the scope of this PR, and so I think this "ugly hack" is ok as a bug fix right now. Medium or long term we should really think over if this admin site thing still makes sense for plugins, modules, packages, and components, if I understand him right. |
|
We already have other ugly hacks for the same reason, see e.g. my PR #25404 . |
|
I don't think that PR was a hack. But even if it was, it's not an excuse to add more hacks. |
|
Depends ... if adding hacks means more consistency because existing code is alrerady full of hacks it could make sense 😄 (attention, sarcastic joke) |
We could do it here. This PR really just moves some existing code around. The code that was initially added for B/C reasons, so we wouldn't have to set prefixes per component. We could remove this code from MVC library and set correct prefixes in components. But this would affect 3rd party components as they would also have to do the same. |
|
I see. |
|
Thanks! Actually spent some time on this and the fix looks cleaner to me than what we had before too |
Pull Request for Issue #23510.
Summary of Changes
This should fix using admin base path in frontend, e.g. when using Editors-XTD plugins.
Testing Instructions
Edit an article in frontend.
In editor, click CMS Content and any button (except Page Break or Read More)
Expected result
Modal with a list of items opens up.
Actual result
Modal with error appears, e.g.:
Documentation Changes Required
Maybe.
@mbabker @laoneo @wilsonge please review.