St/base url normalization #1876
Conversation
- Fix base url when is appended with a script filename - Add default base url http://flarum.local when CLI wizard used - Remove some code duplication - Add minor improvement to the UX when CLI wizard used Fixes flarum#1861
- Update the normalisation method according to the testing suite
|
Hey, thank you for that awesome first contribution, we really appreciate it! ❤️ And it must say, this looks amazing for a first PR, so well done. 👍 That said, please give us a bit of time to look into this. We're pretty busy finalizing the upcoming release (due on Friday) right now. Until then, you could take care of the low-hanging fruit which was reported by StyleCI: https://github.styleci.io/analyses/zeOMDr |
|
Damn, you're fast. 😉 |
|
I am a little bit familiar with the process so I waited for the automated report. :D I need to setup my PhpStorm according to the StyleCI so the auto-format won't cause any more troubles. |
There was a problem hiding this comment.
Okay, since you've been so proactive, let me give you the biggest bit of feedback I have right now.
Could you please implement the base URL normalization in a separate class (a value object) that is then passed to the Installation::baseUrl() method? Similar to what we do for e.g. the database config, so I feel like this would bring the change more in line with the existing code. The class could have multiple constructors like fromString and fromUri, for example. Most importantly, though, it would be the one place where we implement conversion and validation of base URLs in our installer.
In addition, this would then be the class to test with your unit test. Having an InstallerTest is a bit misleading right now, as it only tests the normalization anyway.
It's super cool that you already added tests, by the way. 👍
|
Awesome. Thanks for the feedback. This will be the first thing to do tomorrow. |
Changes: - Add BaseUrl value object - Change test suite - The normalisation method now accepts empty value
After my changes I haven't tried to run the test setup, on my local machine which caused an error during creation of the testing config.php file as it is trying to use var_export() on an object which triggers a magic call to __set_state() which is not defined in the BaseUrl.
|
I hope I got the right idea of the value objects and did it correctly. Although, this approach is very open to different implementations and not sure about correctness/incorrectness. |
# Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit.
- Extract a method for email address generation - Consistent types - No docblocks for types where superfluous - Tweak console output - Don't inherit from integration test's base class in unit test
|
@relax4o Thanks a lot for your awesome PR! I can't believe this is your first open-source contribution - you did really well. Thanks for trying to respect our conventions and sticking to what's there. I pushed a little bit of cleanup in 3417c0c - I sometimes find this easier than doing lots of review iterations. Apart from small things that are just a matter of taste, I mostly removed docblocks that don't add any information where we have typehints (in fact, they had already gotten out of sync with the implementation), and extracted one more method. That's what I like about value objects: they tend to attract logic, which is great for encapsulation and - in this case - reuse. Thanks again! ❤️ |
|
Thanks for accepting my PR. I saw you've changed all my UK style into American style. I'll try to stick with American next time. :D Great additions. Thank you for doing it for me. By the way, it's me relax4o. This has become my official account so I moved everything here. Further comments and PRs will be added from this account. |
- Extract a method for email address generation - Consistent types - No docblocks for types where superfluous - Tweak console output - Don't inherit from integration test's base class in unit test
- Extract a method for email address generation - Consistent types - No docblocks for types where superfluous - Tweak console output - Don't inherit from integration test's base class in unit test
Fixes #1861
Changes proposed in this pull request:
The PR contains a couple of more fixes which were related to the referenced issue.
The Web and CLI installers are a bit more safe to use avoiding unexpected further issues like an incorrect
mail_fromdue to unfulfilled base url via CLI.Reviewers should focus on:
Are you happy with the way I dealt with the issue?
Confirmed
php vendor/bin/phpunit).Note:
This is my first contribution to an open source project ever. I tried to follow the other's ways of doing things. If I did something wrong I'll be more than happy to point it to me so I can get into this faster.