Slim4 #1972
Slim4 #1972
Conversation
There was a problem hiding this comment.
That's really an impressive work! 👏 🚀
I made a few comments, but overall it looks good. I didn't have time to review the tests (but I'm confident seeing they all pass), nor to actually test the changes yet. Thank you for making the PR pretty easy to review by keeping mostly the same format everywhere.
I'm going to open 2 developer experience improvement issues, that shouldn't be handled in this PR to keep the current format and not pile up other changes.
- Improve readability of request accessor, instead of
$object->getArray()['key'] ?? 'default'which is not ideal - Improve DI container reference
|
Thanks for your Code review. I will look how I can integrate it. |
|
@ArthurHoaro I've updated the code. |
|
Is there anything I can do that the code will be accepted? |
|
Hi! Nope thanks, the ball is in my court, I need to do another round of review/testing. |
|
Is there any progress on accepting this PR? |
nodiscc
added
cleanup
There was a problem hiding this comment.
I'm sorry it took me so long to get back to this PR. Life gets busy.
Overall, I don't want to have too many back and forth, so I'm happy with the current state of this PR regarding all code changes (besides my 2 comments).
After testing more or less everything, it seems to be working smoothly, and I didn't encounter any issue with a default PHP server setup. However, trying to run Shaarli in a subfolder failed, and I assume it's because basePath no longer auto-calculated by Slim, and it's expected to be setup manually.
I don't want to add it as a setting, because it would make the setup process more complex, so I'm going to check if I can submit an easy fix in this PR with https://github.com/selective-php/basepath.
We will have to try it with both nginx and Apache before merging it though. I'm afraid it could break some specific setups otherwise.
| @@ -36,6 +36,8 @@ class FileUtils | |||
| */ | |||
| public static function writeFlatDB($file, $content) | |||
| { | |||
| $folder = basename($file); | |||
| file_exists($folder) || $folder !== 'sandbox' || mkdir($folder, 0755, true); | |||
There was a problem hiding this comment.
I'm not sure to understand, but regardless I don't think it's related to Slim4 upgrade, so if it's out of scope I'd rather revert this change.
|
|
||
| // Default extension translation from the current theme | ||
| $themeTransFolder = rtrim($this->conf->get('raintpl_tpl'), '/') . '/' . $this->conf->get('theme') . '/language'; | ||
| $themeTransFolder = __DIR__ . '/../' . rtrim($this->conf->get('raintpl_tpl'), '/') . '/' |
There was a problem hiding this comment.
I'm not convinced that we have to change how we are handling paths for the translator. Not that it's shouldn't be done if there are pros of doing so, but it could introduce bugs and I don't think it affects Slim.
So obviously it didn't work out of the box. I added a quick hack to make it work with Shaarli, it's not ideal but it should do for now. I tried with a real nginx instance + subdirectory, and everything works fine now! The tests need an update and we should be good on that front. |
|
@ArthurHoaro |
|
@Beutlin Whatever is fastest. If you have a moment, go ahead. Otherwise, I should be able to merge the PR by the end of the weekend. |
|
Hi, |
|
Hi, |
I've upgraded the Slim library in Shaarli from version 3 to 4.
The whole API has changed to be compatible with PSR-7. Thus a lot has changed in each file.
See also issue #1967.
Please have a look at it and give appropriate feedback.