convert out of range exception to http exception #60

makasim · 2013-04-26T13:45:22Z

I found that useful to convert OutOfRangeCurrentPageException to NotFoundHttpException.

For example a user can change the page number in the url, he will get 500 page before now it would 404 which is more descriptive.

makasim · 2013-05-14T10:20:16Z

do you plan to merge it? If so I close this PR and a change locally in a project. In case you ok to merge it let'me know what is left.

makasim · 2013-06-26T10:25:27Z

Are you gonna merge it? if not we find another way to solve this problem in the project code. You can just leave few words about PR.

Baachi · 2013-06-26T10:29:27Z

<3

makasim · 2013-10-15T12:00:12Z

any news?

richsage · 2013-10-15T12:08:41Z

This would be a BC break, and it also does not allow for your own business logic to do other tasks based on this specific exception. Thanks for the contribution though :-)

makasim · 2013-10-16T15:17:54Z

@richsage

This would be a BC break

yeah I agree. We could avoid BC breaks. For example the listener could not be enabled(I enabled it in my project code) or I can make it configurable at bundle extension level. It will be disabled by default so any BC breaks.

and it also does not allow for your own business logic to do other tasks based on this specific exception.

that's not really true. Cuz it is easy to add a listener with a higher priority and do what ever you want.

I really think this logic should be inside the bundle. Without the listener it a bit broken. For example I copied the link http://example.com/users?products=10 and send it via email to my friend. He opens this link few days later. If amount of products has been changed during that time he endup with 500 error. No one like 500 and it is clear that 404 is better.

If it is not provided by the bundle I would have to copy\past that listener from one project to another. This is what I and my friends already do.

richsage · 2013-10-16T15:24:08Z

I think if you're able to make the change configurable (and disabled by default as you suggested, I'd be more inclined to include it. Could you adjust accordingly?

makasim · 2013-10-16T15:29:29Z

@richsage sure, I would do it next days

richsage · 2013-10-16T15:30:34Z

Thanks :-)

makasim · 2013-10-17T15:06:56Z

@richsage updated.

richsage · 2013-10-19T16:25:03Z

README.md

@@ -42,6 +42,13 @@ Add the WhiteOctoberPagerfantaBundle to your application kernel:
        );
    }

+That's would also be good to set these options to true:


This needs a bit more explanation of what the configuration options actually do.

@richsage done, please review

Koc · 2013-12-09T19:51:49Z

@makasim what about option for doing redirects to the 1st/last page?

makasim · 2013-12-10T07:38:53Z

@Koc that's a good idea indeed but implementation could be tricky. The route could be defined by a developer and be very custom.

Koc · 2013-12-10T07:52:00Z

We can use all current matched route and get parameters and replace only page number in one of them.

makasim · 2013-12-10T07:54:27Z

@Koc sounds good

makasim · 2014-01-02T10:32:32Z

@richsage done, please review

pablodip · 2014-01-02T10:36:14Z

It looks nice, a couple of suggestions:

A more descriptive name would probably include exception and http, something like convert_not_valid_max_per_page_exception_to_http_not_found_exception. If it's too long, it could be in an index convert_exceptions, and inside max_per_page_to_http_not_found.
Having tests would be nice! ;)

makasim · 2014-01-02T10:47:42Z

Having tests would be nice! ;)

yes it is a good idea, though it could blow the PR since we have to setup test env and staff like that. in any case we would try to add some tests.

If it's too long, it could be in an index convert_exceptions, and inside max_per_page_to_http_not_found.

sounds as a good idea. let'd do this

pablodip · 2014-01-02T10:50:55Z

There is already a kernel (https://github.com/whiteoctober/WhiteOctoberPagerfantaBundle/tree/master/TestsProject/app), but I think we would need to tests both cases (converting and not converting, so different configuration). This could be done maybe through environments?

makasim · 2014-01-02T10:53:06Z

This could be done maybe through environments?

@pablodip maybe we can pass (somehow) an config as array to AppKernel? this way a concrete test can adjust configuration to its needs.

pablodip · 2014-01-02T10:54:57Z

https://github.com/symfony/symfony/blob/master/src/Symfony/Bundle/FrameworkBundle/Test/WebTestCase.php#L171

makasim · 2014-01-02T11:03:23Z

@pablodip squashed&rebased against the latest master. Now we working on @pablodip your comments.

Koc · 2014-01-02T11:20:24Z

Resources/config/pagerfanta.xml

@@ -57,6 +57,14 @@
            <tag name="twig.extension" />
            <argument type="service" id="service_container" />
        </service>
+
+        <service id="pagerfanta.convert_not_valid_max_per_page_to_not_found_listener" class="WhiteOctober\PagerfantaBundle\EventListener\ConvertNotValidMaxPerPageToNotFoundListener">


What about adding public=false?

no, listeners should be public since they lazy loaded,

starting from symfony 2.4 it throw exception with a warning that listener must be public

makasim · 2014-01-03T08:19:12Z

the plan is to introduce next concept(this one would allow to extend it with what @Koc proposed later):

white_october_pagerfanta:
  exceptional_cases:
    page_out_of_range: none|404
    page_invalid: none|404
    # ...

and listener:

<?php

class ConvertExceptionalCaseTo404Listner 
{
    public function setConvertPageOutOfRange($boolean);

    public function setConvertPageInvalid($boolean);

    // ...
}

and in the DI extnesion:

<?php

if ($config['white_october_pagerfanta']['exceptionnal_cases']['page_out_of_range'] == '404') {
    $listenerDefenition->addMethodCall('setConvertPageOutOfRange', array(true));
}

pablodip · 2014-01-03T08:39:52Z

white_october_pagerfanta:
  convert_exceptions:
    out_of_range_page_to_http_not_found: true|false

invalid_page? What's that exception? I think we should explicitly indicate pagerfanta's exceptions.

makasim · 2014-01-03T08:41:31Z

invalid_page? What's that exception? I think we should explicitly indicate pagerfanta's exceptions.

@pablodip this is for cases when page is not a number.

makasim · 2014-01-03T08:43:44Z

@pablodip there are lots of exceptions which start from not. I dont like having config options which starts from not. Though that's ok for exceptions.

pablodip · 2014-01-03T08:48:11Z

I would personally specify the exceptions:

white_october_pagerfanta:
  convert_exceptions:
    out_of_range_page_to_http_not_found: true|false
    not_valid_current_page_to_http_not_found: true|false
    ...

By the way, why is it difficult to implement the redirection? It would be something similar to what we already do when paginating, no?

white_october_pagerfanta:
  exceptions_strategy:
    out_of_range_page: false|to_http_not_found|redirect_to_page_1
    not_valid_current_page_to_http_not_found: false|to_http_not_found|redirect_to_page_1
    ...

Something like this seems nice, but it's up to you as you're implementing it! ;)

Out range exception to http not found

makasim · 2014-01-03T14:05:08Z

@pablodip done, please review

Skadabr · 2014-01-03T14:08:55Z

testOutOfRangeException() and testWrongMaxPerPageException()
print message in stderr
https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpKernel/EventListener/ExceptionListener.php#L99
I have no idea how it catch in tests.

makasim · 2014-01-03T14:13:03Z

I have no idea how it catch in tests.

it could be solved by injecting a logger service. for that we would need a monolog bundle setuped and configured. I dont think its a good idea "just to hide a message".

pablodip · 2014-01-03T20:13:08Z

DependencyInjection/Configuration.php

+                ->arrayNode('exceptions_strategy')
+                    ->addDefaultsIfNotSet()
+                    ->children()
+                        ->scalarNode('out_of_range_page')->defaultValue(self::EXCEPTION_STRATEGY_NONE)->end()


default to false or none?

Ok, sorry, my fault, I didn't see that! ;)

pablodip · 2014-01-03T20:23:34Z

It seems ok for me. I have the suggestions I've told you, but they're suggestions for names or how to get the exceptions when testing. I's also make unit tests for the events. Anyway I'm willing to merge. So just let me know if you wanna do some of the suggestions or prefer to merge like this ;)

makasim · 2014-01-03T20:28:27Z

@pablodip yeap, we address your suggestions tomorrow. let's wait till than, thanks for the review

pablodip · 2014-01-03T20:29:02Z

Nice, thanks! ;)

correct tests names

makasim · 2014-01-04T07:58:15Z

@pablodip done for tests names but we decided to keep action as is. It is a functional test so it would be good to test all things at once.

…p-not-found convert out of range exception to http exception

pablodip · 2014-01-04T10:05:40Z

Thanks! ;)

richsage closed this Oct 15, 2013

richsage reopened this Oct 16, 2013

richsage reviewed Oct 19, 2013
View reviewed changes

makasim and others added 2 commits January 2, 2014 13:01

convert out of range exception to http exception

cf29531

[doc] add better explanation of covert exception parameter.

5f19daa

Koc reviewed Jan 2, 2014
View reviewed changes

default exception tests

3851e66

Skadabr and others added 3 commits January 3, 2014 15:50

change parameters to show 404 exception

2268764

some fixes

46204a8

Merge pull request #2 from Skadabr/out-range-exception-to-http-not-found

db6c9e0

Out range exception to http not found

pablodip reviewed Jan 3, 2014
View reviewed changes

Skadabr and others added 2 commits January 4, 2014 09:54

correct tests names

a2911a7

Merge pull request #3 from Skadabr/out-range-exception-to-http-not-found

e3fece4

correct tests names

pablodip added a commit that referenced this pull request Jan 4, 2014

Merge pull request #60 from formapro-forks/out-range-exception-to-htt…

9446f5b

…p-not-found convert out of range exception to http exception

pablodip merged commit 9446f5b into sampart:master Jan 4, 2014

Koc mentioned this pull request Sep 15, 2014

[RFC] Redirect to last available page strategy #96

Open

convert out of range exception to http exception #60

convert out of range exception to http exception #60

Conversation

makasim commented Apr 26, 2013

makasim commented May 14, 2013

makasim commented Jun 26, 2013

Baachi commented Jun 26, 2013

makasim commented Oct 15, 2013

richsage commented Oct 15, 2013

makasim commented Oct 16, 2013

richsage commented Oct 16, 2013

makasim commented Oct 16, 2013

richsage commented Oct 16, 2013

makasim commented Oct 17, 2013

richsage Oct 19, 2013

Choose a reason for hiding this comment

makasim Jan 2, 2014

Choose a reason for hiding this comment

Koc commented Dec 9, 2013

makasim commented Dec 10, 2013

Koc commented Dec 10, 2013

makasim commented Dec 10, 2013

makasim commented Jan 2, 2014

pablodip commented Jan 2, 2014

makasim commented Jan 2, 2014

pablodip commented Jan 2, 2014

makasim commented Jan 2, 2014

pablodip commented Jan 2, 2014

makasim commented Jan 2, 2014

Koc Jan 2, 2014

Choose a reason for hiding this comment

makasim Jan 2, 2014

Choose a reason for hiding this comment

makasim commented Jan 3, 2014

pablodip commented Jan 3, 2014

makasim commented Jan 3, 2014

makasim commented Jan 3, 2014

pablodip commented Jan 3, 2014

makasim commented Jan 3, 2014

Skadabr commented Jan 3, 2014

makasim commented Jan 3, 2014

pablodip Jan 3, 2014

Choose a reason for hiding this comment

makasim Jan 3, 2014

Choose a reason for hiding this comment

pablodip Jan 3, 2014

Choose a reason for hiding this comment

pablodip commented Jan 3, 2014

makasim commented Jan 3, 2014

pablodip commented Jan 3, 2014

makasim commented Jan 4, 2014

pablodip commented Jan 4, 2014