-
-
Notifications
You must be signed in to change notification settings - Fork 229
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
Stop using long deprecated ParamProcessor stuff #4172
Conversation
8b371d9
to
808e210
Compare
808e210
to
4fc246e
Compare
4fc246e
to
44f0b97
Compare
I put a new version of the "workaround" in place now. I suspect the bounds can just be removed for the limit parameter instead though can't find where they are set in the place. |
@mwjames some more deprecations incoming ;) https://github.com/JeroenDeDauw/ParamProcessor#release-notes |
@mwjames I'm currently writing a new result format so if you have time to review this |
Yeah this is real ugh. I'm kinda forced to use the deprecated stuff in my new code now :( |
Yeah this is real ugh. I'm kinda forced to use the deprecated stuff in my
new code now :(
If someone can confirm that the cited limit issue (#... see above) did
not create an opening for any regression (we don't have any
integration test for this so I cannot verify whether this got broken
or not) then this can be merged.
…On 8/1/19, Jeroen De Dauw ***@***.***> wrote:
Yeah this is real ugh. I'm kinda forced to use the deprecated stuff in my
new code now :(
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#4172 (comment)
|
Just tried it. Seems no change with master for limits exceeding their limit. |
if ( isset( $params['limit'] ) && isset( $parameters['limit'] ) ) { | ||
$parameters['limit']->setValue( (int)$params['limit'] ); | ||
$parameters['limit'] = new ProcessedParam( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I felt particular uneasy about this alteration knowing the limit setting has caused a regression in the past therefore I devised an integration test #4187 that would fail if the limit where causing some unexpected changes due to use of ParamDefinition::setDefault
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As expected from the Hero Of The Tests ;)
This PR is made in reference to: JeroenDeDauw/ParamProcessor#40
Not tested and expecting at least one thing to blow up
ResultPrinter
still usesParam
though that is for another PR