Any endpoint that is calling $c->objects->search after calling $c->req->params->remove('param_name') is having that parameter reinstated to the search object
Created attachment 184155 [details] [review] Bug 40417: Regression test
To reproduce, apply the regression test patch and run the test - it will fail
Created attachment 184160 [details] [review] Bug 40417: Regression test
Created attachment 184161 [details] [review] Bug 40417: Remove 'validation' method
This looks correct, but we need to deal with other things for this not to break existing features. Look at the dependencies I added. BTW. I added ``` use DDP; p($c->validation->output); p($c->req->params->to_hash); ``` at the beginning of `objects.search_rs` and run: ``` prove t/db_dependent/api/v1/query.t ``` and verified one thing I mentioned yesterday: when a query parameter is passed multiple times, the calculated structure is the same (i.e. `$args`). When it is only one, they differ. This might not be a real issue, but it is worth making sure we have tests and it doesn't break anything.
This looks good. I added a PR on the QA tools repo so the QA script doesn't look for `$c->validation` in the test file, as we really want it there for the regression test. Please enhance the commit message.
Created attachment 184212 [details] [review] Bug 40417: Regression test prove t/db_dependent/Koha/REST/Plugin/Objects.t
Created attachment 184213 [details] [review] Bug 40417: Remove 'validation' method We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made Test plan: 1) Apply the regression test patch and run the test 2) Test should fail 3) Apply this patch 4) Run test again 5) Test should pass
Created attachment 184317 [details] [review] Bug 40417: Regression test prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 184318 [details] [review] Bug 40417: Remove 'validation' method We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made Test plan: 1) Apply the regression test patch and run the test 2) Test should fail 3) Apply this patch 4) Run test again 5) Test should pass Signed-off-by: David Nind <david@davidnind.com>
Created attachment 184406 [details] [review] Bug 40417: Regression test prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: David Nind <david@davidnind.com>
Created attachment 184407 [details] [review] Bug 40417: Remove 'validation' method We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made Test plan: 1) Apply the regression test patch and run the test 2) Test should fail 3) Apply this patch 4) Run test again 5) Test should pass Signed-off-by: David Nind <david@davidnind.com>
Created attachment 184409 [details] [review] Bug 40417: Regression test prove t/db_dependent/Koha/REST/Plugin/Objects.t Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 184410 [details] [review] Bug 40417: Remove 'validation' method We shouldn't use the ->validation method anymore as it isn't stable. We need to pass the params into the method accounting for any param changes made Test plan: 1) Apply the regression test patch and run the test 2) Test should fail 3) Apply this patch 4) Run test again 5) Test should pass Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
I PQA this. * Code is correct * Side effects from using ->validation covered by tests * QA script happy (with the linked PR applied locally for the QA test tools) Good catch Matthew and thanks for the patches!
Nice work everyone! Pushed to main for 25.11
Nice work everyone! Pushed to 25.05.x
*** Bug 30526 has been marked as a duplicate of this bug. ***