feat: sync container with Laravel container v9.9.0

saas786 commented 2 years ago

Improve PHP 8 compatibility
add bound, bindIf, make, singletonIf, scoped, scopedIf, tag, tagged, addContextualBinding, when, factory, getClosure, rebinding, refresh, rebound, getReboundCallbacks, wrap, call, makeWith, resolved, isShared, isAlias, removeAbstractAlias, getContextualConcrete, findInContextualBindings, isBuildable, hasParameterOverride, getParameterOverride, getLastParameterOverride, resolvePrimitive, resolveClass, resolveVariadicClass, notInstantiable, unresolvablePrimitive, beforeResolving, resolving, afterResolving, fireBeforeResolvingCallbacks, fireBeforeCallbackArray, fireResolvingCallbacks, fireAfterResolvingCallbacks, getCallbacksForType, fireCallbackArray, getBindings, getAlias, getExtenders, forgetExtenders, dropStaleInstances, forgetInstance, forgetInstances, forgetScopedInstances, flush, getInstance, setInstance functions to container

saas786 commented 2 years ago

Note: https://github.com/saas786/hybrid-core/commit/b04f4a24a1d7276aa966d5c6ebe9cb1acddc81fe

justintadlock commented 2 years ago

My biggest issue right now is that I don't think we need to replicate everything Laravel is doing. If someone needs all those features, it's easier just to plug in the Laravel container. When I initially put the container together, I intentionally wanted something more scaled down for what we needed for themes/plugins.

And, changes should be more iterative. 2,000-lines of code is a bit much. I want us to approach changes with very specific and defined purposes. For example, if there's a need for a Container::boundIf() method, we open a ticket to address that use case.

The current PR is far too large in scope.

If there are still PHP 8+ compatibility concerns, I have a fork of this container in the Blush project that has been tested with 8.0 and 8.1: https://github.com/blush-dev/framework/blob/master/src/Core/Container.php

saas786 commented 2 years ago

My biggest issue right now is that I don't think we need to replicate everything Laravel is doing. If someone needs all those features, it's easier just to plug in the Laravel container. When I initially put the container together, I intentionally wanted something more scaled down for what we needed for themes/plugins.

And, changes should be more iterative. 2,000-lines of code is a bit much. I want us to approach changes with very specific and defined purposes. For example, if there's a need for a Container::boundIf() method, we open a ticket to address that use case.

The current PR is far too large in scope.

If there are still PHP 8+ compatibility concerns, I have a fork of this container in the Blush project that has been tested with 8.0 and 8.1: https://github.com/blush-dev/framework/blob/master/src/Core/Container.php

We are not replicating everything what Laravel does. Its just the container part.

No its not easier to plug in Laravel container, maybe for new project sure, but on existing projects where I am using HC, not its not easier.

Initially it was just about PHP 8 compatibility, but then I decided I should keep it in sync with Laravel Container, so can bring in updates frequently. I have been using Laravel last couple of years, so it becomes weird when I switch between LC & HC.

Like we discussed this before, not many users are using it moving forward, most of them are on either v4, or maybe even v3, and some on v5. V. few are using v6, but I am primarily using it. So I am okay with such change.

Its a one off PR with such amount of changes, moving forward there will be none or smaller PRs if any.

Note: This kind of conflict was the primary reason I asked you to hand over the HC repo, so I can maintain it however I need. Hope it make sense.

lkraav commented 2 years ago

Note: This kind of conflict was the primary reason I asked you to hand over the HC repo, so I can maintain it however I need. Hope it make sense.

Going for a personal fork may make more sense here.

saas786 commented 2 years ago

Note: This kind of conflict was the primary reason I asked you to hand over the HC repo, so I can maintain it however I need. Hope it make sense.

Going for a personal fork may make more sense here.

Yes, need Justin's feedback, if he disagrees, certainly we need to maintain our own fork.

justintadlock commented 2 years ago

I'm saying that I don't want to replicate Laravel's container. HC is not meant to be interchangeable with Laravel, and keeping the two in sync will never be something that's desirable for me. I want to be clear up front about that so that there's no confusion going forward.

I also want to say that I appreciate the work you've put into this. What you've done is no small feat.

With that said, we should always be mindful of the PR contributing guideline of "problem first, solution second": https://github.com/themehybrid/hybrid-core/blob/master/contributing.md#pull-requests

This guideline simply means that the first step is to identify a specific problem (e.g., there is "x" bug that needs fixing or here is this "y" enhancement I want to add). If agreed, then a solution is built.

This PR skips the first part of that guideline. No problem has been identified.

Note: We should probably also clarify in the contributing guidelines that a problem should be tightly scoped.

I am happy to discuss any specific features in their own tickets for adding to HC. I am now and have always been open to that. You mentioned some "weirdness" when switching between projects. Perhaps that is a good starting point for one or more tickets so that we can specifically look at those problems and address them.

Note: This kind of conflict was the primary reason I asked you to hand over the HC repo, so I can maintain it however I need. Hope it make sense.

This kind of conflict is also the reason that my primary role is that of project lead. Someone still needs to make the final call, especially on big changes.

Going for a personal fork may make more sense here.

Even that is unnecessary. If there is specific functionality that's needed, a simple sub-class will work:

class CustomClass extends Application
{
    // custom methods
}

And, this is also a great way to narrow down the scope of this whole thing and figure out what would be great to bring back upstream.

saas786 commented 2 years ago

I know and agree with you on PR(s) guidelines and scope of HC.

As you can easily guess from branch name, I didn't plan this PR initially, it was more about PHP 8 compatibility.

But because of lack of time on my side, and lack of support from community, I decided to give it more time and effort, and sync the two containers, so I don't have to file small PRs for each and every function, and ask dev's to spend time testing over and over again.

Which is evident from this PR, I filled this PR on 10 Jan, 2022, no feedback on this since than.

Anyway most of the functions or almost all are internal functions, only few such as make, when, tagged etc are helper functions.

If I remove all helper functions, there will still be one large PR, which will be the base for future function PRs. So I don't see a benefit to file separate PRs.

I hope you make an exception this time, like you did for v3 to v4, v4 to v5 :D

saas786 commented 2 years ago

changed PR base from master to dev.

saas786 commented 2 years ago

The biggest thing here is that any classes/functions from Laravel need to have a copyright/license (@copyright and @license associated in the doc block). Also, a mention in the readme (master/readme.md#copyright-and-license). Otherwise, it looks good.

Sure, will look into DocBlock license & copyright, and will add Laravel details, instead of our own.

saas786 commented 2 years ago

Merging to dev branch, will work on License updates after some feedback from community.

themehybrid / hybrid-core

feat: sync container with Laravel container v9.9.0 #191