Undeprecate Controller paginate/$paginate?

[AD7six]

Many moons ago, the code for Controller::paginate was moved to a component, and the controller property/function deprecated. I think all would agree that slimming the controller class was a good move, I'm not sure that deprecating the controller property/function is the right move though.

However I don't think that the function paginate or the property $paginate should have been deprecated. It's come up in discussion a few times in cakephp-dev, I think it's worth considering whether the controller paginate logic should be deprecated or not.

To summarize the problem with the code in 2.3 right now - this doesn't work:

    <?php

    class FoosController extends Controller { // <- deliberate to indicate that $paginate is not set

        function index() {

            $this->paginate['conditions']['is_active'] = 1; // notice: modification of overloaded property has no effect

            $this->paginate();

        }

    }

There are other permutations/problems, but that for me is the most common annoyance. Some things to consider:

• Convenience
• Not needing to load a class (the paginator component) just to set the default pagination settings
• Backwards compatibility
• Merging mess and/or Controller::$paginate and PaginatorComponent::$settings being inconsistent

My position is that I think the controller property, and the controller paginate wrapper are fine. The fact that the function calls a component to do the work is just an implementation detail. As such I think it is appropriate for the controller function to read it's own config, and the paginate if it's called explicilty to read it's own config. In it's simplest form, I'd like to suggest the following:

https://gist.github.com/AD7six/6069103

Alternatively, and especially if the controller properties remain deprecated, Jippi came up with the following:

https://gist.github.com/jippi/40565a21c21ebafbaf5f

I'm "cool" with the controller property/function remaining deprecated - but is it what we want?

Opinions on a postcard please.

AD

[Ravage84]

Sorry, not a big fan of postcards. You couldn't read my handwriting anyway ;-P

As we discussed it today in different context (https://github.com/cakephp/docs/pull/653#issuecomment-21484822) the removal of the function/property won't happen before 3.0 anyway.
This means using the old code should/will work in any past, current and future 2.x version.

But what about the future, about 3.0? How should the whole thing look and work there?
If we want to keep or at least support them until 3.0, we don't have to deprecate them now.

I'm not against it either but I think the inconsitency is an issue because even third party plugins started to adapt, which let's the user apps become even more inconsistent in the end:
https://github.com/CakeDC/search/commit/94560ca929a89d328100f5a761df85a0cb38d91d
That's why I'm for a decision soon, a clear communication what the decision is and the needed steps to get consistent again.

Anyway, if their deprecation gets reverted the following two things need to be done at least:
1. Revert this merged PR:
https://github.com/cakephp/cakephp/pull/1443/files
2. Close this ticket as invalid/wont-fix:
https://cakephp.lighthouseapp.com/projects/43067/tickets/138-cookbook-mentions-deprecated-paginate-in-several-places

On the other hand if the deprecation stays we should resolve this ticket
https://cakephp.lighthouseapp.com/projects/43067/tickets/138-cookbook-mentions-deprecated-paginate-in-several-places
By either change the docs to the new way only or show both ways side by side and recommend the undeprecated one.

Ciao
Marc

[José Lorenzo]

I really like that the paginator lives in a separate component, but it has proven to me to be a pain to configure. Specially as configs are not merged or inherited as with other variables in the controller.

My main problem is the inability to set globaly the paramType as querystring without repeating myself in every controller when I change the paginator settings. In that, I agree that undeprecating $paginate makes sense, while keeping the paginator settings separate of the conditions.

What I would definitely like to see is the undeprecation of the paginate() method, as it is a nice way of lazy loading the component and a method that is extremely easy to mock in unit tests.

Additionally it could make sense to expose a method in the Paginator component to add options or settings instead of using an array directly, that way we could have more control on where it is stored and how it can be changed.

--
You received this message because you are subscribed to the Google Groups "cakephp-core" group.
To unsubscribe from this group and stop receiving emails from it, send an email to cakephp-core...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

[AD7six]

On Thursday, 25 July 2013 01:13:13 UTC+2, Ravage84 wrote:

Sorry, not a big fan of postcards. You couldn't read my handwriting anyway ;-P

As we discussed it today in different context (https://github.com/cakephp/docs/pull/653#issuecomment-21484822) the removal of the function/property won't happen before 3.0 anyway. 

 

This means using the old code should/will work in any past, current and future 2.x version.

The context/problem is different though: The controller paginate functionality won't be removed, but currently it's known to be half broken and not being fixed because "it's deprecated, so users shouldn't be using it anyway".

I mentioned one way in which updating Controller::$paginate doesn't work in the first post, but there are many permutations, and there are infact 3 ways to define pagination settings right now - in the components array, in the controller paginate property and by modifying the settings of the Paginator component. I don't for example think it should be necessary to load the Paginator component in all actions (beforeFilter/because it's in the Components array) _just_ to be able to define the default pagination settings which apply to multiple controller actions.

It's also simply convenient.

I agree with your observed consequences :)

AD

[Ravage84]

This is so true. I was asking me this too recently.
Lazy loading for the win! So I think we can at least undeprecate paginate()...
and add a new function to the controller for configuring the compontent which is a substitute for the currently deprecated controller property.

Marc

[mark_story]

See, I dislike Controller::paginate() it has a few problems in my mind:

* Half of its parameters get sucked in from a property. Invoking it gives you no clue as to what it is going to do.

* It is propped up by 3 different ways to configure it. There should be at most 2. Ideally 1.

* It is a snowflake. While it is convenient in testing to have controller methods load and proxy a component, it is the only component with such magic and I feel it expands the features of Controller well beyond its purpose. To me controllers should be handling request + response data and delegating to more informed objects to do actual work.

I agree that there should be a better, more singular way to define, and update pagination settings. If the proxy method is useful, perhaps we should explore ways that components can add new mixin methods to controllers as a more formal feature of the framework?

-Mark

[AD7six]

On Sunday, 28 July 2013 03:29:17 UTC+2, mark_story wrote:

See, I dislike Controller::paginate() it has a few problems in my mind:

* Half of its parameters get sucked in from a property. Invoking it gives you no clue as to what it is going to do.

* It is propped up by 3 different ways to configure it. There should be at most 2. Ideally 1.

* It is a snowflake. While it is convenient in testing to have controller methods load and proxy a component, it is the only component with such magic and I feel it expands the features of Controller well beyond its purpose. To me controllers should be handling request + response data and delegating to more informed objects to do actual work.

I agree that there should be a better, more singular way to define, and update pagination settings. If the proxy method is useful, perhaps we should explore ways that components can add new mixin methods to controllers as a more formal feature of the framework?

On Sunday, 28 July 2013 02:01:35 UTC+2, mark_story wrote:

I think the method and property are both bad ideas in the current form. While the deprecation and current implementation are not ideal in 2.x. I don't think Controller::paginate() should exist in 3.0, and we should instead explore alternative ways of providing the same features.

I don't think the Controller method + property should exist because they expand the scope of the controller class beyond what it should be doing. Controllers should be mediating request + response cycles and delegating to more informed objects via composition. I think PaginatorComponent is that more informed object, but right now the interface is a bit lacking. Stabbing indexes into its settings array is less than ideal, and we probably should have a method based way of defining and augmenting settings for pagination.

As for the present problems I think removing the overloading on __get() makes the most sense as it restores how the feature should have worked, and how people expect it to work based on all the examples, blog posts etc.

Deciding how to go forwards wont fix that it's bc-broken/unpredictable in 2.x - I feel that still needs addressing.

That said, great points. Especially the snowflake (which I've deliberated on). 

So lets focus on solutions, what solution could we use to address:

• Being able to define pagination defaults per controller without loading the pagination component?
• a convenient way of calling the paginate function from a controller, honoring  any predefined defaults?
  
• a convenient way of calling the paginate function from a controller overriding any predefined defaults/explicitly defining the config to use?

Cheers,

AD

[Renan Gonçalves]

> Being able to define pagination defaults per controller without loading the pagination component?

> a convenient way of calling the paginate function from a controller, honoring  any predefined defaults?

> a convenient way of calling the paginate function from a controller overriding any predefined defaults/explicitly defining the config to use?

For 3.x I would love to see how it would be use traits for it.

We would lose the lazy attachment, but the other two points fits perfect into a trait.

AD

--

You received this message because you are subscribed to the Google Groups "cakephp-core" group.

To unsubscribe from this group and stop receiving emails from it, send an email to cakephp-core...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.
 
 

--
Renan Gonçalves aka renan.saddam
Software Engineer at TrueServer B.V.
CakePHP Core Developer

W: renangoncalves.com

T: twitter.com/renan_saddam

M: +31 (0)6 4662 1860

L: Amsterdam, The Netherlands

[mark_story]

On Sunday, 28 July 2013 08:45:12 UTC-4, AD7six wrote:

Deciding how to go forwards wont fix that it's bc-broken/unpredictable in 2.x - I feel that still needs addressing.

That said, great points. Especially the snowflake (which I've deliberated on). 

So lets focus on solutions, what solution could we use to address:

• Being able to define pagination defaults per controller without loading the pagination component?
• a convenient way of calling the paginate function from a controller, honoring  any predefined defaults?
  
• a convenient way of calling the paginate function from a controller overriding any predefined defaults/explicitly defining the config to use?

Cheers,

AD

I'm all for fixing the current mess and making things work more like they did in 1.x for the remainder of 2.x. I don't think we can adequately solve the deprecations with method/property overloading. For 3.0 I think we can explore the issue further when someone has time to do it. You've hit a number of important points that would need to be covered by any replacement solution.

-Mark