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

[4.0] Remove the New MVC classes #18526

Closed
wants to merge 10 commits into from

Conversation

laoneo
Copy link
Member

@laoneo laoneo commented Nov 8, 2017

As with the merge of the new installer #17964, no classes in core use the "New MVC" library anymore. FOF got removed in #17687, so it is time to remove the "New MVC" as well. Like then we have one way to create a component in joomla, using the namespaced MVC classes.

If interfaces are needed on a later point, then we can introduce them ad hoc but for now all interfaces are removed as well.

The class JViewGenericdataexception got namespaced as it is widely used in the core extensions.

…e/new-mvc

# Conflicts:
#	tests/unit/suites/libraries/joomla/model/JModelBaseTest.php
#	tests/unit/suites/libraries/joomla/model/stubs/tbase.php
@mbabker
Copy link
Contributor

mbabker commented Nov 8, 2017

So, to be honest. These classes aren't deprecated yet. And I don't know if I'm comfortable adding deprecations to 3.9 for immediate removal in 4.0.

Plus, this completely breaks patch tester because it is using this MVC implementation, but ¯\_(ツ)_/¯ I guess, just another change I have to make in that component to isolate it from being affected by core changes.

@brianteeman
Copy link
Contributor

im with michael, deprecating in 3.9 is risky and potentially painful

@laoneo
Copy link
Member Author

laoneo commented Nov 8, 2017

As general rule I agree and would not make it. But the New MVC was never fully adopted by the extension developers. I don't know of one extension using it. It even leads to more confusion for new devs because it is not documented and not used in core since the migration of com_config and the new installer. IMO with Joomla 4 we have the chance to remove some legacy stuff and the New MVC is one of them. Carrying a not used library trough the Joomla 4 life cycle is for me a no go.

We can deprecate it with 3.8.3 if we need a longer deprecate time frame.

@asika32764
Copy link
Contributor

Windwalker RAD and Asikart extensions all uses new MVC now
https://github.com/ventoviro/windwalker-joomla-rad/blob/staging/src/Controller/Controller.php#L27

@brianteeman
Copy link
Contributor

As @mbabker said com_patchtester uses it.. the reality is we have no idea who is using it. Even if you searched the JED and found none you dont know about all the custom code out there.

@mbabker
Copy link
Contributor

mbabker commented Nov 8, 2017

We can deprecate it with 3.8.3 if we need a longer deprecate time frame.

SemVer doesn't allow deprecations in a patch release so we should only do that if it's determined it is absolutely critical.

@laoneo
Copy link
Member Author

laoneo commented Nov 8, 2017

If it is really the wish to keep it in core, then I can close this pr, no problem. But again for me it is bad to keep two MVC libs (where one is not maintained anymore) in core when we ship a new major version.

@asika32764
Copy link
Contributor

asika32764 commented Nov 8, 2017

Actually I use new MVC for years and I think it is not a bad design, it has some better features that legacy MVC can not reach.

But if core team decided not to use it in the future, I will also agree that don't keep it in the code base of J!4. It's easy for me to create a base MVC classes to replace it for my extensions.

A solution is that we can make these classes as an official library, every developers who needs it can simply include in their code and decouple with Joomla core.

@laoneo
Copy link
Member Author

laoneo commented Nov 8, 2017

The framework contains already libs for it https://github.com/joomla-framework?q=mvc.

@mbabker
Copy link
Contributor

mbabker commented Nov 8, 2017

Ya but it is impossible to write code using "new MVC" and the Framework packages in a compatible manner. So for those using it you're giving them the hardest B/C break out of all the breaks going into 4.0 because the only way to mitigate is to abstract away and not use core at all.

The two MVC layers have two different approaches. One is a fully opinionated implementation with CRUD already in place. The other is a clean slate with just some helper methods for common things. It's not that we are supporting two MVC implementations in core honestly. If you break it down one is littered full of practices that aren't good (directly reading request in models for starters), not supportive of DI constructs and relying heavily on global singletons while the other lets you use best practice if you're willing to put in a bit more leg work. So I don't think it's necessarily bad that we have the two differing base implementations as one is a more opinionated setup and probably the closest thing core offers to RAD while the other gives you as the dev full control over it all.

And just to add to the "this uses new MVC" list, the controllers for the downloads site API use it (using FOF3 models since the requests are all reading ARS data).

@laoneo
Copy link
Member Author

laoneo commented Nov 8, 2017

Just thought it is a common agreement to remove the New MVC to slim down the core a bit. Doesn't look like. Don't have the energy to start a new battle.

@laoneo laoneo closed this Nov 8, 2017
@laoneo laoneo deleted the j4/remove/new-mvc branch November 8, 2017 14:17
@asika32764
Copy link
Contributor

If we won't remove it, can we have a Joomla\Platform namespace to place these classes?

@mbabker
Copy link
Contributor

mbabker commented Nov 8, 2017

So my argument right now is mainly based on two things:

  • Too short a deprecation notice for 4.0
  • An inability to easily migrate code using these MVC classes
    • As a subpoint to this it isn't possible to migrate to the Framework packages due to various incompatibilities (typehinting and not extending the right classes for things like Input or DatabaseDriver)

As these are empty classes for the most part and low maintenance, I don't feel like there's an urgent need to drop them with 4.0, otherwise we could also argue to drop the MVC packages in the Framework too for the same reasons being suggested for the CMS. At this point, I'd suggest marking them deprecated now and drop in 5.0 if we really want to drop them (for the MC part of that it'll be much easier to use the Framework packages in 4.x, the view part will probably require a custom HTML base class not using the Framework's Renderer).

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.

None yet

5 participants