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

Stripping of trailing slashes breaks stuff #24

Open
michaelarnauts opened this issue Sep 9, 2019 · 12 comments
Open

Stripping of trailing slashes breaks stuff #24

michaelarnauts opened this issue Sep 9, 2019 · 12 comments

Comments

@michaelarnauts
Copy link

Related to #22 and #12

It seems the recent changes in routing regarding the stripping of slashes broke a few things. I also learned that Kodi doesn't like urls without a trailing slash when they point to a directory listing. See xbmc/xbmc#16516 (comment)
Stripping them seems like a bad idea then. They won't work when bookmarked since the last part gets stripped of if it doesn't end with a slash.

IMO, this should be the right behavior then:

  • @plugin.route('/tvshows/category/<category>/'): plugin.url_for() should include the slash
  • @plugin.route('/tvshows/category/<category>'): plugin.url_for() should not include the slash
  • a route to /tvshows/category/ should end up on the function with the @plugin.route('/tvshows/category/<category>/') decorator
  • a route to /tvshows/category should end up on the function with the @plugin.route('/tvshows/category/<category>') decorator

I guess this was the behaviour of 0.2.0.

The one thing that could be added is that:

  • a route to /tvshows/category could also end up on the function with the @plugin.route('/tvshows/category/<category>/') decorator
  • a route to /tvshows/category/ should also end up on the function with the @plugin.route('/tvshows/category/<category>') decorator

@tamland and @dagwieers what do you think?

@michaelarnauts
Copy link
Author

michaelarnauts commented Sep 9, 2019

With the current behavior:

  • @plugin.route('/tvshows/category/<category>/'): plugin.url_for() doesn't include the slash anymore

This breaks bookmarking.

@dagwieers
Copy link
Contributor

@michaelarnauts That was exactly what I intended with #12, the flexibility ought to be there during matching (only).

That said xbmc/xbmc#16516 is a bug, and should be fixed as one.

@da3dsoul
Copy link

I was about to post this. I don't update Kodi very often, since doing so always breaks my stuff, and waiting for a bug fix in Kodi core can be like waiting for Half-Life 3, anyway.

@tamland
Copy link
Owner

tamland commented Sep 15, 2019

What's the 'stuff' and the 'things' that broke, exactly? Tested with one of my own plugins and couldn't find any problems with favorites..

@michaelarnauts
Copy link
Author

Check the 2nd comment. If you add a folder to your bookmarks, it will not work without a trailing slash, but the new routing version strips all trailing slashes.

@tamland
Copy link
Owner

tamland commented Sep 15, 2019

By bookmarks you mean favorites? Adding folders works fine here..

@michaelarnauts
Copy link
Author

Sorry, I indeed mean favourites. If you have an addon with a path that ends in a number, it will not work. The route gets generated without a trailing slash now, and those don't work in Kodi when used as a favorite or in activatewindow.

@michaelarnauts
Copy link
Author

Check xbmc/xbmc#16516 (comment) the issue is explained there.

@mediaminister
Copy link

mediaminister commented Oct 6, 2019

VRT NU add-on is also hit by this when using a path identifier.( @plugin.route('/url/<path:url>'))

Stripping of a trailing slash means changing the variable in this case. This should never have happened!

It is completely ridiculous that I have to add the trailing slash again in my code after the routing plugin has just removed the trailing slash.

@da3dsoul
Copy link

da3dsoul commented Oct 7, 2019

Personally, I just think it was a bad idea. As far as URLs go in general, '/stop' != '/stop/'

@dagwieers
Copy link
Contributor

@da3dsoul For web-servers, /stop matches /stop/, if /stop (the file entry) was not found. And this is how I wanted to see it implemented: #22 (comment)

So stripping the slashes always is IMO wrong behavior.

@da3dsoul
Copy link

da3dsoul commented Oct 7, 2019

Indeed. That is hopefully going to fix things

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

No branches or pull requests

5 participants