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

Generic correct merging of local params #939

Merged
merged 10 commits into from
Jul 5, 2021

Conversation

thomascorthals
Copy link
Member

@thomascorthals thomascorthals commented Jun 28, 2021

I took the idea from #937 and applied it to AbstractRequestBuilder::renderLocalParams() instead. That should catch all cases where a user puts local params in a query and we try to add more in our code.

I hit a snag with converting code that uses LocalParameters::render() though and I would like feedback from @wickedOne. The way parameters are stored and rendered within LocalParameters isn't compatible with the key => value array renderLocalParams() expects. I have included a kludge to parse them into such an array for now as a proof-of-concept. It works as expected: all tests pass and the output from the examples is identical.

Would it be ok to remove LocalParameters::render() completely? Or does it add something that I'm missing?


Also added array support to Helper::qparser() so it handles ['excludeTags' => ['tag1', 'tag2']] the same way as AbstractRequestBuilder::renderLocalParams() does.

@codecov
Copy link

codecov bot commented Jun 28, 2021

Codecov Report

Merging #939 (82b6319) into master (08b839f) will increase coverage by 0.01%.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #939      +/-   ##
==========================================
+ Coverage   92.60%   92.62%   +0.01%     
==========================================
  Files         334      334              
  Lines        8223     8243      +20     
==========================================
+ Hits         7615     7635      +20     
  Misses        608      608              
Flag Coverage Δ
unittests 92.62% <100.00%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Component/RequestBuilder/FacetSet.php 100.00% <100.00%> (ø)
src/Component/RequestBuilder/Stats.php 100.00% <100.00%> (ø)
src/Component/RequestBuilder/SubRequest.php 75.00% <100.00%> (ø)
src/Core/Query/AbstractRequestBuilder.php 100.00% <100.00%> (ø)
src/Core/Query/Helper.php 100.00% <100.00%> (ø)
src/Core/Query/LocalParameters/LocalParameters.php 100.00% <100.00%> (ø)
src/QueryType/Select/RequestBuilder.php 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 08b839f...82b6319. Read the comment docs.

@thomascorthals thomascorthals changed the title Localparams Generic correct merging of local params Jun 28, 2021
@mkalkbrenner mkalkbrenner marked this pull request as ready for review July 1, 2021 06:30
@mkalkbrenner mkalkbrenner self-requested a review July 1, 2021 06:31
mkalkbrenner
mkalkbrenner previously approved these changes Jul 1, 2021
Copy link
Member

@mkalkbrenner mkalkbrenner left a comment

Choose a reason for hiding this comment

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

The patch looks good to me.

@mkalkbrenner
Copy link
Member

@wickedOne @thomascorthals Should I merge the PR as it is or should we wait for another review?

@thomascorthals
Copy link
Member Author

@mkalkbrenner I still had to remove LocalParameters::render() but that meant rewriting all the unit tests for LocalParameters. I wanted to be sure it was worth the effort first. 😉 Also cleaned up the code to get the actual parameters for rendering, my POC used a kludge for that.

Noticed that booleans weren't yet converted to 'true' and 'false' and that 0 wouldn't be rendered while it is a valid value. Fixed those as well.

@mkalkbrenner mkalkbrenner merged commit 2493816 into solariumphp:master Jul 5, 2021
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.

2 participants