[3.x] Compatibility with Laravel 9

[erikgaal]

There were some more changes required before getting the package to work with Laravel 9. I've added testing on L9 to the workflow so we can be sure it's compatible before we merge.

Thank you @CodeSkills for the work in your PR. I've made sure to add you as a co-author for the commit regarding the FilesystemTenancyBootstrapper.

[stancl]

Thanks for the PRs! I'm happy to merge #800 or #802, whichever adds the L9 support fully

With regards to the workflow changes you made, I think it'll fail now since Laravel 9 requires PHP 8. And it's trying to use it with PHP 7.4.

It'd probably be best to merge #793 first. There are a few changes remaining there, so if you could try implementing them, we'll be able to see how this PR works.

I just responded to a few reviews there.

[codecov-commenter]

Codecov Report

Merging #802 (e1ab850) into 3.x (9c79267) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##                3.x     #802      +/-   ##
============================================
+ Coverage     87.32%   87.33%   +0.01%     
- Complexity      383      384       +1     
============================================
  Files           103      103              
  Lines          1128     1129       +1     
============================================
+ Hits            985      986       +1     
  Misses          143      143              

Impacted Files	Coverage Δ
...rc/Bootstrappers/FilesystemTenancyBootstrapper.php	100.00% <100.00%> (ø)
src/Middleware/CheckTenantForMaintenanceMode.php	80.00% <100.00%> (+7.27%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c79267...e1ab850. Read the comment docs.

[stancl]

@CodeSkills could you explain the changes a bit?

[stancl]

I'd like to make sure this uses the root overrides correctly, as described here https://tenancyforlaravel.com/docs/v2/filesystem-tenancy/

I think I might add some assertions to the BootstrapperTest, assuming the PR can be set up locally

[erikgaal]

I think this was an IDE suggestion to negate the condition and remove the original if part. Because the original if part of the conditional (the setPathPrefix call) was no longer needed.

[CodeSkills]

It is just a simplification, it works the same way as before, since there is $this->app['config']["filesystems.disks.{$disk}.root"] = $root; at the end of the function. You dont have to do this $filesystemDisk->getAdapter()->setPathPrefix($finalPrefix = $root); because it changes the config itself.

[stancl]

Pushed those test changes now

[stancl]

I'd rewrite the if to not include the negation and assignment in the condition. You can make them separate

[CodeSkills]

You can rewrite it to something like this, to make it more clear:

$root = str_replace(
    '%storage_path%',
    storage_path(),
    $this->app['config']["tenancy.filesystem.root_override.{$disk}"] ?? ''
);
// Default root
if (!$root) {
    $root = "{$root}/{$suffix}";
}

[erikgaal]

I've refactored the proposed changes to prevent overwriting used variables. It took me a while to fully understand what the code was doing, but this should clear it up.

[erikgaal]

Looks good to me now :)

[stancl]

Do you think we can finish 793 as I wrote here? #802 (comment)

[colinmackinlay]

Do you think we can finish 793 as I wrote here? #802 (comment)

@stancl - would be great if this could be merged now if it is working. Even if you don't tag a release until #793 is done we could still pull it in via dev:master and run Laravel 9 upgrades :)

[stancl]

@colinmackinlay I can merge the PR sooner and tag a version after all of the other things are resolved, but there are still some reviews here that should be addressed before merging.

[SupianIDz]

@colinmackinlay I can merge the PR sooner and tag a version after all of the other things are resolved, but there are still some reviews here that should be addressed before merging.

is there any progress ? We have entered the tenancy stage of our program. I hope it's merged soon.

Thank you all for your hard work ❤️ .

[magarrent]

Hey! It's the package already compatible with L9? Or there are some pending issues? Maybe I can help

[stancl]

Would appreciate @CodeSkills' feedback on the review where I mentioned him, and a refactor of this https://github.com/archtechx/tenancy/pull/802/files/f5269b2b025e24243cba78e06435f25bfff9b3fa#diff-a7131a9582938c1b642f3d517a9064cbacb7b83d75fb52fe2c6e52dbca26a584

[CodeSkills]

I have written there an update on how you could rewrite it. Hope it helps! (Someone should probably test it before since it has been changed from my PR, when somebody replaced the vars with {$root}, which I did not test.

[oxiginedev]

hey! @stancl , please what's the update on support for laravel 9. Any help needed?

[stancl]

I'll probably finish and merge this later today 👍🏻

[stancl]

Thanks for the $finalPrefix refactor (as well as your work on all this) @erikgaal! It looks great now.

The only thing I'm unsure about is the review re: S3, but I'll merge this into 3.x anyway. I'd appreciate if people who use S3 could test that branch and report if there are any issues. If there are not, I'll tag a release tomorrow. I'll also try to revisit the logic tomorrow (in code, not prod) to see if it's okay.

So to use the dev branch locally, you can just run:

composer require stancl/tenancy:3.x-dev

And then switch it back to ^3.0 once this is released.

[tkuhnert]

I just updated locally to L9 using 3.x-dev. Using flysystem-aws-s3-v3 in combination with a minio server. Everything works as expected so far. 👍

[stancl]

Thanks a lot for the testing!

[jtomek]

Hi, I also tested it on Vapor. All worked fine except the filesystem root url. I have described the issue and a hotfix here: #739 (comment)

[stancl]

@jtomek Can you confirm that this regression was introduced in the latest release (and therefore it all worked fine in 3.5.1)?

[jtomek]

@stancl, I will try my best to find a slot for it. This week is rather busy. I can share that I have been using and updating your package regularly for a long time and the issue at hand started with the Laravel 9 update.

[miketimeturner]

@jtomek Can you confirm that this regression was introduced in the latest release (and therefore it all worked fine in 3.5.1)?

I can confirm this is the case. I have the same issue and therefor tenant directories now reside in directory "/" this is also having an impact retrieving the media and you are met with a permission denied, probably due to an incorrect url.

The issue no longer exist after reverting to 3.5.1

[stancl]

Thanks for the testing! I'll try to get it fixed this week.

[trippo]

Any news on this?
Are now production-ready v.3.5.2?

[stancl]

See the release notes

[trippo]

Thanks seem that v3.5.5 solves all bugs.

Thanks