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

Process all Shaarli pages through Slim controllers #1511

Merged
merged 61 commits into from
Aug 27, 2020

Conversation

ArthurHoaro
Copy link
Member

What is this?

All Shaarli pages are know process through Slim framework controllers. It means:

  • Improved code quality, with better encapsulation, dependency injection, and we're no longer manipulating global variables everywhere.
  • Proper routing system, meaning better URLs
  • 2k+ LoC in index.php no longer exists
  • The entire code base is now unit tested πŸŽ‰

That's also the last piece of major refactoring that was planned for Shaarli.
It means that we will be able to focus on new features later, and in a safer way thanks to unit tests.

Demo

Please feel free to test it here (login demo/demo): https://shaarli-next.hoa.ro

In the meantime, you can use https://shaarli-current.hoa.ro/ if you need to compare behaviours.

Here is an export of the demo links if you need to delete/re-import existing links: https://gist.github.com/ArthurHoaro/853243091d5f296d897907c52dae8f1a

Routing

Besides everything that changed under the hood, the major change here is new URLs. I chose the new routes one by one over the past few months, so they're is definitely room for improvement.

If you think that a route should changed or improved, please submit your suggestion now, as it will be way harder to change it later.

Existing public routes have been mostly preserved (using a redirection to the new route), but most admin ones have been dropped. The reasoning behind this it that we don't want to break external permalinks, while admin ones are not public anyway.

Route Legacy New Preserved
Installation N/A /install N/A
Permalink /?{hash} /shaare/{hash} βœ…
Login /?do=login /login ❌
Picture Wall /?do=picwall /picture-wall βœ…
Tags Cloud /?do=tagcloud /tags/cloud βœ…
Tags List /?do=taglist /tags/list βœ…
Daily /?do=daily /daily βœ…
Daily RSS /?do=dailyrss /daily-rss βœ…
Atom Feed /?do=atom /feed/atom βœ…
RSS Feed /?do=rss /feed/rss βœ…
OpenSearch /?do=opensearch /open-search βœ…
Add tag to search /?do=addtag /add-tag/{tag} ❌
Remove tag from search /?do=removetag /remove-tag/{tag} ❌
Logout /?do=logout /logout ❌
Tools /?do=tools /admin/tools ❌
Change password /?do=changepasswd /admin/password ❌
Configure /?do=configure /admin/configure ❌
Add Shaare /?do=addlink /admin/add-shaare βœ…
Post /?post /admin/shaare βœ…
Edit Shaare /?edit_link /admin/shaare/{id} ❌
Delete Shaare /?delete_link /admin/shaare/{id}/delete ❌
Export /?do=export /admin/export ❌
Import /?do=import /admin/import ❌
Thumbnails Update /?do=thumbs_update /admin/thumbnails ❌

I have omitted routes which are only redirecting to other routes.

Performances

Don't worry, Shaarli is still fast, that's why we chose a micro-framework.

Current master (1000 requests, per batch of 10 concurrent requests)

Document Path:          /
Document Length:        120768 bytes

Concurrency Level:      10
Time taken for tests:   21.830 seconds
Complete requests:      1000
Failed requests:        0
Total transferred:      121222000 bytes
HTML transferred:       120768000 bytes
Requests per second:    45.81 [#/sec] (mean)
Time per request:       218.296 [ms] (mean)
Time per request:       21.830 [ms] (mean, across all concurrent requests)
Transfer rate:          5422.96 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       59   66   9.7     63     147
Processing:   115  150  31.1    143     401   <-------- This line is the most important (in ms)
Waiting:       94  124  30.2    117     380
Total:        178  216  33.8    208     548

With Slim (1000 requests, per batch of 10 concurrent requests)

Document Path:          /
Document Length:        121057 bytes

Concurrency Level:      10
Time taken for tests:   21.614 seconds
Complete requests:      1000
Failed requests:        0
Total transferred:      121535000 bytes
HTML transferred:       121057000 bytes
Requests per second:    46.27 [#/sec] (mean)
Time per request:       216.139 [ms] (mean)
Time per request:       21.614 [ms] (mean, across all concurrent requests)
Transfer rate:          5491.22 [Kbytes/sec] received

Connection Times (ms)
              min  mean[+/-sd] median   max
Connect:       59   67  10.5     63     129
Processing:   109  147  29.7    139     397 <-------- This line is the most important (in ms)
Waiting:       87  122  28.6    114     374
Total:        168  214  31.5    207     506

3rd party

Themes

Unfortunately, there is quite a few breaking changes for 3rd party theme maintainers.

  • Path: see URL changes in the previous section
  • Path: all paths MUST be prefixed with {$base_path}
  • Path: all assets include MUST be prefixed with {$asset_path}
  • Daily RSS: the whole feed is now processed through template. Please update yours using the default template.
  • Export : the export form is no longer a method=GET form.
  • Links per page: linksperpage input name is now nb
  • Alerts: you should support user messages across all pages using $global_errors, $global_warnings, $global_successes.

Plugins

Router class is no longer supported and must be replaced by TemplatePage (or LegacyRouter).

Release

This contains A LOT of refactoring. I will release a BETA version of Shaarli to make it clear that it still requires some testing.

I will probably merge this in a couple of weeks, if that's OK with everyone.

And update dependencies and tests.

Note that SESSION['tags'] has been removed a log ago
The daily RSS template has been entirely rewritten to handle the whole feed through the template engine.
Also it was missing on the default template feeds
Including:
  - visibility
  - links per page
  - untagged only
  - Reorganize visitor controllers
  - Fix redirection with Slim's requests base path
  - Fix daily links
@nodiscc
Copy link
Member

nodiscc commented Aug 24, 2020

Shaarlier is able to use the REST API so this should keep working. But it also supports username/password-based logins which will stop working due to ?do=login being removed

@mro
Copy link

mro commented Aug 24, 2020

@nodiscc

stop working due to ?do=login being removed

ShaarliOS just follows the redirects from ?post= to find and fill the login form and has no other hard-coded routes. https://github.com/mro/ShaarliOS/blob/master/swift4/ShaarliOS/ShaarliHtmlClient.swift#L278 - the mandatory form fields need stable names.

Please confirm this process is supposed to continue to work.

@nodiscc
Copy link
Member

nodiscc commented Aug 24, 2020

Please confirm this process is supposed to continue to work.

It should work. Or maybe not :) I don't have an iOS device to test. Can you check that the app works against https://shaarli-next.hoa.ro/ ?

@dimtion
Copy link

dimtion commented Aug 25, 2020

Hi guys,

I'm amazed by this wonderful work, kudos to everybody involved!

I've just tested Shaarlier against https://shaarli-next.hoa.ro/ and as expected, it works well with the API. I won't make any update to support the new /login path, but Shaarlier will keep username/password authentication for compatibility with older instances.

@ArthurHoaro
Copy link
Member Author

Thanks for the feedback!

As I'm using Shaarli dockerized, is/will there a beta docker image available that I could swap out like shaarli/shaarli:slim-beta in the FROM clause? It would make testing and refactoring things a lot easier.

For tests purposes you can currently use arthurhoaro/shaarli:next. You will be able to use shaarli/shaarli:master when this PR is merged. And I'm not exactly sure yet, but if I release a beta version I will probably add a beta image.

@mro Most previous routes has been kept, including ?post=, you just need to support URL redirection.

@ArthurHoaro ArthurHoaro merged commit af41d5a into shaarli:master Aug 27, 2020
@ArthurHoaro ArthurHoaro deleted the wip-slim-routing branch August 27, 2020 08:27
@ilesinge
Copy link

shaarli-related has been fixed, in a retro-compatible fashion. Thanks for the heads up.

@immanuelfodor
Copy link

Thanks for being first @ilesinge and for the elegant solution of not breaking older Shaarlies, I'll use your PR as reference for fixing my (probably) broken plugins at the weekend :) https://github.com/ilesinge/shaarli-related/pull/4/files

@ilesinge
Copy link

@immanuelfodor The code is not so clean, mostly straightforward given the time I had, but yes, the compatibility is important. You may also be interested in checking ilesinge/shaarli-related#5 (thanks @ArthurHoaro )
Unless someone finds another solution, I'll keep using old URLs in this plugin and relying on the redirects to be sure older installs aren't broken.

@immanuelfodor
Copy link

How long will the LegacyRouter be supported? Years maybe? :)

@immanuelfodor
Copy link

immanuelfodor commented Aug 30, 2020

I think I've found a bug.

Here I need to check if I'm on an edit page or not: https://github.com/immanuelfodor/shaarli-markdown-toolbar/blob/slim-v0.12.0-beta/markdown_toolbar/markdown_toolbar.php#L92 However, this condition is never true as the LegacyRouter contains an edit_link value but the $data['_PAGE_'] is containing an editlink value if I var_dump it one line above.

Is this undocumented change intentional? If yes, I should strip the underscores from any router constants and data page values and check against the stripped values to keep compatibility with older Shaarlies.

array(4) {
  ["_PAGE_"]=>
  string(8) "editlink"
  ["_LOGGEDIN_"]=>
  bool(true)
  ["_BASE_PATH_"]=>
  string(0) ""
  ["css_files"]=>
  array(1) {
    [0]=>
    string(29) "plugins/favicons/favicons.css"
  }
}

Edit: Fixed the above link after pushing the strip underscores logic. It should work on my end whether it is a bug or not in Shaarli.

@immanuelfodor
Copy link

I just wanted to report back that all my plugins are now compatible with the 0.12.0-beta release (latest master), I'm using the new version from now on.

Also released two new plugins in the meantime, see PR #1528

  • A simple custom CSS editor on the plugin admin UI, as until the Material theme is fixed, I'm using the default theme, so I needed a way to customize it to look more Material-ish :D Its readme includes my custom rules as an example.
  • Resurrected the old Emojione plugin as it needed too much bash sed magic in the Dockerfile (How to add plugins via Docker?Β #1372 (comment)), it was not compatible with even the previous namespace changes, but I like to use these emojis in my notes :)

@kalvn
Copy link

kalvn commented Aug 30, 2020

Hi there, on the way to migrate the Material theme, I noticed an issue with the untagged links filter which redirects to <base url>/untagged-only which leads to error 500. I guess something was forgotten here.

@ArthurHoaro
Copy link
Member Author

I think I've found a bug. [...] Is this undocumented change intentional?

No you're right it wasn't intentional. I will change it to use LegacyRouter here, because we don't need unnecessary breaking changes.

I just wanted to report back that all my plugins are now compatible with the 0.12.0-beta release (latest master), I'm using the new version from now on.

I'm glad to hear that, and good job for the new plugins πŸ‘

Hi there, on the way to migrate the Material theme, I noticed an issue with the untagged links filter which redirects to /untagged-only which leads to error 500. I guess something was forgotten here.

Thanks for the report, I will open a dedicated issue.

@mro
Copy link

mro commented Aug 31, 2020

https://shaarli-next.hoa.ro/?post=https://github.com/mro/ShaarliOS/issues/47 redirects to the login form as expected but after login does not redirect back to present the `linkformΒ΄.

@ArthurHoaro can you fix the redirect?

@mro
Copy link

mro commented Aug 31, 2020

@ArthurHoaro and the https://shaarli-next.hoa.ro/?do=configure is important for ShaarliOS, because it looks for the name of the shaarli inside the <form><input>. Could you also redirect that to login and back?

@ArthurHoaro
Copy link
Member Author

Thanks for the report, I have open a PR to fix this redirection.
And I can easily re-add a redirection to the configure page if it's used externally.

@mro
Copy link

mro commented Sep 3, 2020

@ArthurHoaro
Copy link
Member Author

@mro I have switched that instance to the root Docker image shaarli/shaarli:master and redeployed.

@mro
Copy link

mro commented Sep 3, 2020

thanks a lot, things improve but there's still an issue I'm examining at https://github.com/mro/ShaarliOS/issues/47

@mro
Copy link

mro commented Sep 3, 2020

@ArthurHoaro uh, and url fragments seem to get lost along the way, so

https://shaarli-next.hoa.ro/?post=https://github.com/mro/ShaarliOS/issues/47%23i-get-lost

end up without #i-get-lost

@ArthurHoaro
Copy link
Member Author

Good catch, it should be fixed by #1541 (deployed).

@mro
Copy link

mro commented Sep 4, 2020

@ArthurHoaro I think I have another one https://shaarli-next.hoa.ro/?post=http://idlewords.com/talks/website_obesity.htm?foo%3Db+ar%23minimalism unescapes the + to which not only changes the url but renders it invalid.

P.S.: what happens to the form action /admin/shaare when hosted in a subdir?

@ArthurHoaro
Copy link
Member Author

P.S.: what happens to the form action /admin/shaare when hosted in a subdir?

It should work properly and redirect using the subdir. Please open a separate issue if you encounter any other bug. It'll be easier to handle.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet