Keep protocol options in path databases #14168
Conversation
yol
added
Type: Fix
yol
force-pushed
the
db-split-protocoloptions
branch
from
67f57ef
to
baf75bc
Compare
in addition to query options. Does not make much sense to remove the query but keep protocol options. This is needed for video/music database to function correctly as they do not expect protocol options to turn up in the filename.
URIUtils::Split now removes protocol options, and not adding them to the path would cause them to get lost when the path from the database is used directly.
URIUtils::Split now removes protocol options, and not adding them to the path would cause them to get lost when the path from the database is used directly.
yol
force-pushed
the
db-split-protocoloptions
branch
from
baf75bc
to
1edd0f1
Compare
|
This in effect reverses the change made by @koying in #12962 (Only remove options for actual URL in URIUtils::Split ) that was done to fix Trac ticket https://trac.kodi.tv/ticket/17627 So can we discuss what we are doing with stored path and whether it has options or not before we "just try". That PR broke other things that have been fixed but took a while to find |
|
"Just try" is the only option to find and spot the 100 000 of workarounds already in the codebase to have it sane once and for all. |
|
If the consensus (from discussion on Slack) is that URL options be stored - as after all where else are they going to come from - and that #12962 was a mistake, then wouldn't a better solution be to restore how |
|
Before #12962 we were storing options in path, hence this change and that PR seem closely related to me. I just wanted everyone to be aware of the related previous change and issues when reviewing this. If we want to do store options in path again, and that does seem reasonable (but with more testing than just running the limited test cases with have) , then why not do it in URIUtils::Split instead of adding extra method? |
#12962 seems completely unrelated to protocol options to me. Are you maybe confusing query options ( |
|
Yes I am lumping all options (query + protocol) together, sorry if insufficiently nuanced. Having two PR changing what gets stored in the databases (one more obviously than the other), and both questioning if there could be regressions etc., was enough for me to want it at least considered together. It would help greatly if we had a documented db design, rather than just colum name, to go on (for example does path include options or not, what kind or where else are they stored). But we are where we are, and Spiff's memory is the great asset (if only we could use Google to search it) If you have looked at the other PR then fair enough. |
|
Yeah as far as I can see the other PR is orthogonal. Thanks for bringing it up though, really difficult to keep all of this stuff in mind. |
|
Guess we'll just have to try this, @MartijnKaijser said he's fine with it |
|
I would have chosen the opposite: always strip options. Note that path is actually the ID of an item. With this change all info of an item is lost if some options are changed. |
Then where do you get them from when playing an item?
Yes. Same as if you would change the path. Certainly not nice, but do you know a way around it? Theoretically we could try to put protocol options into an extra column. Not sure it's worth the trouble though. |
|
I don't see a problem in storing the protocol options (or any others) in the path string. However we do need to ensure that when file item processing is trying to match items it only uses the appropriate part of this full path. |
Not verifying any certificates renders SSL meaningless. cUrl should have a stock of default CA certificates provided by the system/distributor.
The code was inconsistent as to whether protocol options that may be present in source URIs are written to the paths table in the database.
Mostly there were two paths, one with and one without protocol options, leading to duplicated data.
This patch tries to fix this by settling on the version with protocol options at all times.
I hope this doesn't break too much other stuff, can't say really :-/