Upgrade Illuminate components to 6.x

[franzliedke]

Let's adopt the LTS release. See previous attempt in #1930.

Challenges

• [x] Application contract has gathered even more cruft, let's decouple (#2117, #2142, #2155)
• [x] Dispatcher::fire has become Dispatcher::dispatch (f4ab6f4b1f5fb2f254b7fb65fd924ff65c3c1993)
  • impacts extensions, can we add a BC layer?
• [x] Gate contract now has several unnecessary methods, consider alternatives (#2181)
• [x] Translator contract added more methods, is the contract needed?

[franzliedke]

Status update: I'm almost done with getting rid of Laravel's Application contract. That will take us to version 5.8.

The Gate and Translator stuff are changing in L6 if I am not mistaken.

[askvortsov1]

I'm likely misunderstanding here, but when I was working on the policy/model visibility scoping refactor, it seemed to me that we're really only using a very small part of Gate. Do we want to keep using it at all? It seems like we could make a gate-inspired system with much less overhead

[franzliedke]

That's a good point, but I would rather see that happen with or after the policy rework. No need to block the Laravel upgrade with such a big change to such a central place.

[luceos]

Yeah let's keep the changes for the upgrade to a minimum. @franzliedke Gate and Translator indeed where blocking the upgrade, I'd suggest implementing the required changes and exchange them with simpler components if possible before stable.

[franzliedke]

Made a little more progress with this. Moving this to Beta.14 now.

A status update:

• For the upgrade to Laravel 5.8, the biggest remaining blocker is avoiding Laravel's huge Application contract. I made small steps here with the goal of injecting it in far fewer places.
• I have prepared a commit that splits up the Application, most importantly:
  • ignore the Application contract, we don't need it
  • stop inheriting from and instead inject a Container instance
  • separate paths / URLs from the config derived from config.php, from the application bootstrap stuff
  • get rid of some of the fluff, such as deferred service providers and other Laravel-specific things
• I will PR this commit (an improved version of it, really) at the beginning of the B.14 cycle, so that we have enough time to find / encounter issues caused by this change.
• The Gate change (for Laravel 6) might become obsolete with the policy changes in #2056.

[luceos]

Would it be feasible to remove the Application contract and its implementation completely? Just using a Container implementation makes a lot more sense in the scope of our targeted code logic.

[franzliedke]

Yes, I listed that point above. :smiley:

[franzliedke]

@luceos I managed to finish the PR that splits up Application and container: #2142. Is that what you had in mind?

[luceos]

@franzliedke i meant fully removing the application including contract... 😇

[askvortsov1]

Isn't that what the PR does though? Remove the application contract?

[askvortsov1]

Also, as per Franz's earlier message, I am not envisioning us keeping Gate after the policy refactor, so that will go too.

[franzliedke]

Yes, indeed. I removed the contract. The class is still there, but significantly slower smaller (container gone, several contract methods gone). We may have to revisit the structure of Application <-> App(Interface) <-> Site(Interface), but most of this is internal, so not extremely urgent.

Maybe we just need to figure out better names, the concepts themselves have been useful in my eyes.

[askvortsov1]

Gate is gone with #2181. For translator, as we actually use it (unlike gate), and that interface is commonly used, I'd say we should keep the interface. For events dispatcher, we can provide a BC layer for a few releases, and then drop that.

^^^ Is this more-or-less an accurate representation of the work left to be done (after #2155 is figured out and merged)?

[franzliedke]

For events dispatcher, we can provide a BC layer for a few releases, and then drop that.

Core usages were actually removed quite a while ago in https://github.com/flarum/core/commit/f4ab6f4b1f5fb2f254b7fb65fd924ff65c3c1993, then the method itself was dropped for real when upgrading to Laravel 5.8. No BC planned here. (I might have a list of usages in community extensions laying around somewhere thanks to @clarkwinkelmann, but it wasn't very long.)

For translator, as we actually use it (unlike gate), and that interface is commonly used, I'd say we should keep the interface.

Totally agree.

Is this more-or-less an accurate representation of the work left to be done?

According to what was noted in this issue and related PRs so far, yes. I will carefully go through the L6 upgrade guide to see whether there's anything else. But that's the next step, then we're done. :clinking_glasses:

[franzliedke]

Went through the list and found the following things that need to be changed when upgrading to L6:

• Translator contracts (docs, diff) :scream: - we may have to consider something here for backwards compatibility.
• :heavy_check_mark: The Mandrill mail driver will no longer be shipped by Laravel core, so we can not offer it. Clean up core / translations and leave it to flagrow/mail-drivers.
• :shrug: The default queue retry limit changed - we could mention that in the upgrade guide, but it's probably more relevant for @luceos' redis queue extension.
• :heavy_check_mark: In somewhat related news, I noticed we're not explicitly requiring the nesbot/carbon package, but we're implicitly using it (installed through illuminate/support) - should be added to our Composer requirements.

[askvortsov1]

• Translator contracts (docs, diff) scream - we may have to consider something here for backwards compatibility.

We could just add those methods as deprecated to the Locale\Translator.php object? And remove them in stable? Also make that change on the frontend translator as well?

• leave it to flagrow/mail-drivers.

That repo doesn't seem to be maintained, but at the very least we should make it clear in the release announcements to avoid a situation like with SES. Also, perhaps FoF might be interested in picking it up?

• In somewhat related news, I noticed we're not explicitly requiring the nesbot/carbon package, but we're implicitly using it (installed through illuminate/support) - should be added to our Composer requirements.

Good catch! And maybe that'll solve the annoying deprecation warnings too :D