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

Promote use of addViewClasses() instead of overriding viewClasses() #7747

Merged
merged 2 commits into from
Oct 13, 2023

Conversation

ADmad
Copy link
Member

@ADmad ADmad commented Oct 11, 2023

No description provided.

@ADmad ADmad added this to the 5.x milestone Oct 11, 2023
@LordSimal
Copy link
Contributor

I'd say both are valid options but I don't know which one we should recommend as a default

@ADmad
Copy link
Member Author

ADmad commented Oct 11, 2023

Method overriding as a default doesn't make sense when we already have the API to achieve the same result with a method call.

@LordSimal
Copy link
Contributor

By this logic we shouldn't overwrite/extend the initialize() method of the Base controller inside the AppController as well.

Besides, your docs are missing a parent::initialize() method call here as well. Otherwise, all components loaded in the AppController wouldn't work in that specific controller (and would confuse users).

If we go this way then (imho) we should remove the viewClasses() method to not have different ways of doing the same thing in the same class which is not really feasible right now since we just released Cake5.

@ADmad
Copy link
Member Author

ADmad commented Oct 11, 2023

By this logic we shouldn't overwrite/extend the initialize() method of the Base controller inside the AppController as well.

That's an apples to oranges comparison since initialize() isn't for one specialized task and there's no other alternative to overriding it.

Besides, your docs are missing a parent::initialize() method call here as well. Otherwise, all components loaded in the AppController wouldn't work in that specific controller (and would confuse users).

I can idiot proof the examples.

If we go this way then (imho) we should remove the viewClasses() method to not have different ways of doing the same thing in the same class which is not really feasible right now since we just released Cake5.

There's no need to remove it, just not documenting overriding it is enough. You can't prevent users from overriding any other controller method anyway.

@LordSimal
Copy link
Contributor

Technically one could overwrite the constructor of the controller to add "initialize" logic to it but having a simpler initialize method with no arguments is definitely a bit easier to understand.

In the end I am fine with either approaches, I just prefer the current way of using viewClasses() since we just introduced this method (and I did a separate video on youtube on it) so users don't get even more confused besides all the Cake5, Chronos and Migrations/Phinx stuff.

@ADmad
Copy link
Member Author

ADmad commented Oct 11, 2023

The problem with viewClasses() overriding was that one couldn't change the classes from outside the controller, for e.g. a component or custom implementation like action classes of the Crud plugin, which was possible with RequestHandler.

That's why addViewClasses() was added later and it's the better API.

@ADmad
Copy link
Member Author

ADmad commented Oct 11, 2023

Technically one could overwrite the constructor of the controller to add "initialize" logic to it but having a simpler initialize method with no arguments is definitely a bit easier to understand.

And similarly using addViewClasses() is a lot more convenient than method overriding and it also mitigates the problem I stated above.

Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Looks good once the calls to parent::initialize() are added.

@markstory markstory merged commit 250a81d into 5.x Oct 13, 2023
3 checks passed
@markstory markstory deleted the ADmad-patch-1 branch October 13, 2023 02:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants