Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 3 commits into from
Jun 21, 2022

Conversation

saas786
Copy link
Contributor

@saas786 saas786 commented Jan 10, 2022

  • 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
Copy link
Contributor Author

saas786 commented Apr 25, 2022

Note: saas786@b04f4a2

@saas786 saas786 force-pushed the syed/compat/php8 branch 2 times, most recently from 330bcb8 to a5bbd73 Compare April 25, 2022 09:20
@saas786 saas786 marked this pull request as ready for review April 25, 2022 09:29
@saas786 saas786 changed the title feat: upgrade container - in progress feat: sync container with Laravel container v9.9.0 Apr 25, 2022
@justintadlock
Copy link
Member

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
Copy link
Contributor Author

saas786 commented Apr 26, 2022

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.

@saas786 saas786 force-pushed the syed/compat/php8 branch 2 times, most recently from 7c3d835 to 47d9cb8 Compare April 26, 2022 08:13
- add bound, bindIf, make, singletonIf, scoped, scopedIf, tag, tagged, addContextualBinding, when, factory, when, 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

- Improved PHP 8 compatiblity
- Sync the HC container with Laravel container v9.9.0

Note: Keep psr/container in sync with WooCommerce psr/container version.
@lkraav
Copy link

lkraav commented Apr 26, 2022

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
Copy link
Contributor Author

saas786 commented Apr 26, 2022

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
Copy link
Member

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
Copy link
Contributor Author

saas786 commented Apr 27, 2022

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 saas786 changed the base branch from master to dev May 23, 2022 06:43
@saas786
Copy link
Contributor Author

saas786 commented May 23, 2022

changed PR base from master to dev.

Copy link
Member

@justintadlock justintadlock left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (https://github.com/themehybrid/hybrid-core/blob/master/readme.md#copyright-and-license). Otherwise, it looks good.

@saas786
Copy link
Contributor Author

saas786 commented Jun 20, 2022

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
Copy link
Contributor Author

saas786 commented Jun 21, 2022

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

@saas786 saas786 merged commit 08a218d into themehybrid:dev Jun 21, 2022
@saas786 saas786 deleted the syed/compat/php8 branch June 21, 2022 08:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants