3.x Set default locale via Middleware

[iwasherefirst2]

See #921

[jordyvanderhaegen]

Hi @iwasherefirst2, great work so far! 👍
I've gone ahead and shared my thoughts on the proposed changes.

Update the entire README

Sounds good, let's delay entire rewrites until 3.x is almost ready though.

Update tests

I've made some changes myself:

• Macro registering does not throw exceptions whilst running tests anymore. This was caused by the macro trying to register itself multiple times. We now check if it has been registered previously, otherwise we discard it.
• Tests now use testbench's default testcase instead of the browserkit one.
• Tests now register routes using the new localized macro.
• Tests now do not refresh the application anymore, as this should not be necessary
  with the new way of registering routes.
• Tests now use get instead of call, which should reduce the amount of code to get the same result.

Some tests are still failing, but that seems to be related to the changes in this PR and should be addressed.

Create a Docker image to run the tests locally (would be very helpful)

Is there a reason you would implement docker here? Doesn't seem necessary to me.
We already have tests running through GitHub Actions for any PR's targetting master.

Perform smoke tests for each feature

Some tests will have to be changed in order to match the new functionality. I wouldn't mind looking into this.

[iwasherefirst2]

• browserkit

Hi @iwasherefirst2, great work so far! 👍 I've gone ahead and shared my thoughts on the proposed changes.

Update the entire README

Sounds good, let's delay entire rewrites until 3.x is almost ready though.

Sure

Update tests

I've made some changes myself:

* Macro registering does not throw exceptions whilst running tests anymore. This was caused by the macro trying to register itself multiple times. We now check if it has been registered previously, otherwise we discard it.

* Tests now use testbench's default testcase instead of the browserkit one.

* Tests now register routes using the new `localized` macro.

* Tests now do not refresh the application anymore, as this should not be necessary
  with the new way of registering routes.

* Tests now use `get` instead of `call`, which should reduce the amount of code to get the same result.

These are all great changes!

Some tests are still failing, but that seems to be related to the changes in this PR and should be addressed.

I also had to make one change after your commits. I realized that in version 2.x, localized routes without a locale in the URL always worked, even when hideDefaultLocaleInURL was disabled. I'm not sure if this was intended behavior or just a side effect/bug. In addition, I added some validation for the {locale} parameter.

In my opinion, routes within the localized group should not be accessible without a locale if hideDefaultLocaleInURL is disabled. Allowing this could result in the language being determined by the browser's language settings (without redirect), leading to inconsistencies for bots, which SEO algorithms typically penalize. Since we're now in 3.x, it might be a good opportunity to revisit and adjust this behavior. What do you think? For now, I’ve reverted the behavior to match 2.x for consistency, but I don’t think it makes much sense.

Create a Docker image to run the tests locally (would be very helpful)

Is there a reason you would implement docker here? Doesn't seem necessary to me. We already have tests running through GitHub Actions for any PR's targetting master.

My local PHP version doesn’t have all the libraries required to run the Orchestra test suite. Currently, I’ve set up the package inside a fresh Laravel application, so I use Sail to run the tests, and it works fine for me now.

That said, I think having a Docker image could still be helpful for contributors. It would allow them to run tests locally without relying on the GitHub runner, which can save time during development. However, it’s not essential, and if you’d prefer not to use it, that’s fine. If I find the time, I might add a Dockerfile and some instructions to the README for future use.

Perform smoke tests for each feature

Some tests will have to be changed in order to match the new functionality. I wouldn't mind looking into this.

Yes, it looks like it.

[iwasherefirst2]

@jordyvanderhaegen It seems that implementing the translated routes feature is a bit tricky with the new approach. I have an idea that I'd like to propose in a separate MR.

Would it make sense to merge this for now and continue iterating on separate branches? This way, it’ll be easier to review changes and avoid conflicts between commits.

The 3 failing tests are al related to translated routes

[jordyvanderhaegen]

@iwasherefirst2

In my opinion, routes within the localized group should not be accessible without a locale if hideDefaultLocaleInURL is disabled. Allowing this could result in the language being determined by the browser's language settings (without redirect), leading to inconsistencies for bots, which SEO algorithms typically penalize. Since we're now in 3.x, it might be a good opportunity to revisit and adjust this behavior. What do you think? For now, I’ve reverted the behavior to match 2.x for consistency, but I don’t think it makes much sense.

I agree with you on this, this doesn't make much sense at first glance. If there's no tests that covered this functionality, let's consider it a bug / unexpected behaviour. Can we make sure to provide tests to ensure this works as expected?

My local PHP version doesn’t have all the libraries required to run the Orchestra test suite. Currently, I’ve set up the package inside a fresh Laravel application, so I use Sail to run the tests, and it works fine for me now. That said, I think having a Docker image could still be helpful for contributors. It would allow them to run tests locally without relying on the GitHub runner, which can save time during development. However, it’s not essential, and if you’d prefer not to use it, that’s fine. If I find the time, I might add a Dockerfile and some instructions to the README for future use.

I see, that's a bit cumbersome. Feel free to tag me in the PR if you'd like a review.

Would it make sense to merge this for now and continue iterating on separate branches? This way, it’ll be easier to review changes and avoid conflicts between commits.

Sure, 3.x is very much a WIP.

[jordyvanderhaegen]

@iwasherefirst2 I'm going to merge this, but won't delete the branch yet. If you have open changes on this branch, you can directly merge them in 3.x.

[iwasherefirst2]

Thanks for merging @jordyvanderhaegen , I just added one commit for the visibility of constructor properties.

[jordyvanderhaegen]

@iwasherefirst2 great, no point in keeping the remote branch then, I deleted it 😄