Closed (fixed)
Project:
Search API
Version:
8.x-1.x-dev
Component:
Framework
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Issue tags:
Reporter:
Created:
26 Feb 2017 at 13:05 UTC
Updated:
16 Apr 2017 at 16:15 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #2
strykaizerJust using the testbots
Comment #3
borisson_Comment #5
borisson_Sorry, too eager here.
Comment #6
strykaizerJust here for the test, ignore
Comment #7
strykaizerComment #9
strykaizerComment #10
strykaizerComment #11
strykaizerReady for reviews now ;)
Comment #12
borisson_Looks great. Thanks.
Comment #13
drunken monkeyThanks for reporting this issue! I was not aware of this problem, it indeed sounds tricky to resolve. I don't see where in the code
Url::fromUserInput()would invoke that hook, but I'm gonna trust you on that.However, there's still several problems both with the idea and the patch (the latter easier to resolve, of course – see my attached revision):
NULL, notFALSE. Core also seems to slightly lean in that direction, although – unsurprisingly – they are more ambiguous on the topic than the Search API.ViewsTest– sorry about that! (If we don't commit a patch here, I'll just have to think of fixing these regardless.)Why do you even need to get the displays' URLs in
processInbound()? Isn't there separate configuration for Pretty Paths for each display? If there is, you should just add the path to that, translating from the search display's returned URL.There's about as many classes with
getUrl()as withgetPath(), so that's not an argument.Comment #15
marthinal commentedAs discussed with @drunken_monkey we can keep ::getUrl() and add a new method ::addPath().
Comment #16
marthinal commentedComment #17
borisson_If we add ::getPath, we should also add tests for that method.
Comment #18
marthinal commentedNeeds work then. Thanks Joris
Comment #19
marthinal commentedComment #20
marthinal commentedComment #21
marthinal commentedAdding tests for ::getPath again
Comment #23
marthinal commentedComment #24
drunken monkeyGreat job, thanks!
Especially good that we figured out this issue with block displays having paths! Views logic strikes again …
I think some of that code was unnecessary workarounds for that?
Also, I think we should try to explain what the difference is between
getPath()andgetUrl(). Maybe we should even deprecate the latter? (I don't think having GET parameters as part of a search's base path is really a use case, and if it were, part of the code (i.e., facets, at least with pretty paths) couldn't support it anyways.) I'd just leave it in now for the stable release, seeing as we're really late in Beta.Then, now that we have
getPath(), we should also use it internally in the display plugin base class for getting the path.Finally, there was a white-space error, as I pointed out to you in person already.
Patch attached, which would be RTBC as far as I'm concerned. Please test/review!
Comment #25
borisson_Woooo!
Comment #26
borisson_I think deprecating
getUrlis a good idea.Comment #27
drunken monkeyOK, needed a re-roll anyways.
Will commit if tests pass.
Comment #29
drunken monkeyOK, great.
Committed.
Thanks again, everyone here!