themehybrid / hybrid-core

Official repository for the Hybrid Core WordPress development framework.
GNU General Public License v2.0
689 stars 144 forks source link

Allowing for multiple boot sequences across plugins/themes #183

Closed justintadlock closed 2 years ago

justintadlock commented 3 years ago

I've been thinking through this problem a few times. One of the approaches I thought we should try is allowing for multiple instances of Application. While it was intended to be a single-instance object, it shouldn't necessarily have to be.

What I'm thinking is that plugins/themes should be able to load up a new app and boot it separately:

$slug = new Application( $slug );

$slug->boot();

We'd need to figure out the Proxy system, which is what really makes Application behave similarly to a singleton.


The other solution might be to continue using a single Application instance but provide a "reboot" system that will will run the boot sequence for bindings added after the initial boot. Something like:

$slug = app() instanceof Application::class ? app() : new Application();

$slug->boot(); // would have some logic to only boot newly-added stuff.

I think this second solution would be the least disruptive to HC's code. But, plugin/theme creators would need to run the conditional on their end.

justintadlock commented 3 years ago

Marking this for the 6.0 milestone because I think it's something we can squeeze in. But, this is not a hard deadline. We can pull it for a 7.0 release (any method is likely to break back-compat).

saas786 commented 3 years ago

Thinking out loud:

There are several moving parts I see here.

1) $app = new Application 2) Proxy to $app like Hybrid\App

Reference https://github.com/themehybrid/hybrid-core/blob/bf5ff55f94d5f79f4a47a3e1c5170e0ba0642f5a/src/Core/Application.php#L98 https://github.com/themehybrid/hybrid-core/blob/bf5ff55f94d5f79f4a47a3e1c5170e0ba0642f5a/src/Proxies/App.php#L33

3) Helper function to access $app instance Hybrid\app()

Reference https://github.com/themehybrid/hybrid-core/blob/bf5ff55f94d5f79f4a47a3e1c5170e0ba0642f5a/src/functions-helpers.php#L17 https://github.com/themehybrid/hybrid-core/blob/bf5ff55f94d5f79f4a47a3e1c5170e0ba0642f5a/src/functions-helpers.php#L34

And for provider setup, by default we access it like:

4) $this->app

So ServiceProvider is also referencing hardcoded reference to $app

https://github.com/themehybrid/hybrid-tools/blob/0154820b08bee5d2f0931d5169c361b35536fbfb/src/ServiceProvider.php#L35


So first approach seems like a better fit for me.

It accepts a unique slug for container instance, which can be accessed via $this->uniqueslug and falls back to default $this->app if not provided.

Can we the proxy config dynamic? https://github.com/themehybrid/hybrid-core/blob/bf5ff55f94d5f79f4a47a3e1c5170e0ba0642f5a/src/Core/Application.php#L121

So we can setup accessor and alias via contructor params I guess?

saas786 commented 3 years ago

This is how I am currently using it, and it seems to work.

Initiate the Application instance... https://github.com/saas786/cxl-blocks/blob/8356db82c611cf1770661df64ca6eb42b19dc60c/src/bootstrap-app.php#L18

Custom Application class with few mods... https://github.com/saas786/cxl-blocks/blob/8356db82c611cf1770661df64ca6eb42b19dc60c/src/Core/Application.php

Custom Proxy classes... https://github.com/saas786/cxl-blocks/tree/8356db82c611cf1770661df64ca6eb42b19dc60c/src/Proxies

and Custom ServiceProvider class... https://github.com/saas786/cxl-blocks/blob/8356db82c611cf1770661df64ca6eb42b19dc60c/src/Tools/ServiceProvider.php

it works for me at least...

But it creates duplicate code...

saas786 commented 3 years ago

I have tried extending the Hybrid Core classes like Application, Proxy classes and Service providers, but it didn't work.

What seems to work is: if I still use custom Proxy classes, Extend ServiceProvider classes and modify the Hybrid Core Application class to receive custom configs (see attachment)...

ServiceProvider

namespace CXL\Blocks\Tools;

use Hybrid\Contracts\Core\Application;
use Hybrid\Tools\ServiceProvider as HCServiceProvider;

/**
 * Service provider class.
 *
 * @since  5.0.0
 * @access public
 */
abstract class ServiceProvider extends HCServiceProvider {

    /**
     * Application instance. Sub-classes should use this property to access
     * the application (container) to add, remove, or resolve bindings.
     *
     * @since  5.0.0
     * @access protected
     * @var    Application
     */
    protected $cxl_blocks;

    /**
     * Accepts the application and sets it to the `$app` property.
     *
     * @since  5.0.0
     * @access public
     * @param  Application  $cxl_blocks
     * @return void
     */
    public function __construct( Application $cxl_blocks ) {

        $this->cxl_blocks = $cxl_blocks;
    }
}

And setup new instance like:

$cxl_blocks = new \Hybrid\Core\Application(
    [
        'abstract' => 'cxl_blocks',
        'proxy_class_name' => Custom\Proxies\App::class,
        'proxy_alias' => '\CXL\Blocks\Plugin\App'
    ]
);

Custom\Proxies\Proxy::setContainer( $cxl_blocks );

hybrid-core-configs

This way I am only able to Avoid creating Custom Application class. But other classes I will need...

justintadlock commented 3 years ago

I wanted to approach this from a single-application viewpoint. The Application class was always meant to only be used once, so I thought it best to continue down that route if possible.

Test branch: https://github.com/themehybrid/hybrid-core/tree/6.0-try-one-app

See commit for changes: https://github.com/themehybrid/hybrid-core/commit/31f89e3d0189

My approach to this keeps things really minimal in terms of code changes. Each plugin/theme can boot up the app from whatever point they want. So, we can essentially have multiple boot sequences, but everything still works on a single instance of the app.

Basic bootstrapping for a plugin/theme:

$slug = \Hybrid\booted() ? \Hybrid\app() : new \Hybrid\Core\Application();

All we're doing here is checking the new booted() function. If the app is booted, just reference it with app(). Else, create a new Application instance.

Test code for multiple boots:

$test_a = \Hybrid\booted() ? \Hybrid\app() : new \Hybrid\Core\Application();
$test_a->boot();

$test_b = \Hybrid\booted() ? \Hybrid\app() : new \Hybrid\Core\Application();
$test_b->boot();

$test_c = \Hybrid\booted() ? \Hybrid\app() : new \Hybrid\Core\Application();
$test_c->boot();
justintadlock commented 3 years ago

I also wanted to note that going this route doesn't preclude us from building something that would allow multiple Application classes in the future. It's just the path of least resistance.

justintadlock commented 3 years ago

Resolving this issue should also take care of my use case of HYBRID_DIR. I'm pretty sure we could close out #181.

lkraav commented 3 years ago

Allowing for multiple boot sequences across plugins/themes

Can someone clarify what's the win - what is someone trying to do when they need multiple boot sequences?

saas786 commented 3 years ago

Can someone clarify what's the win - what is someone trying to do when they need multiple boot sequences?

We are not after multiple boot sequences, but rather its a solution to a problem we face when using Hybrid-Core in multiple projects running on same website.

Which ever $slug->boo() is called first, it initiates the framework, which is not ideal and breaks the code. As we lose access to certain singletons etc needed somewhere in other project. Because theme is going to initiate it last most probably, so container is reset for it.

As it can be used in Plugin(s) and Theme at the same time.

So allowing for multiple boot() calls, each one can just work alongside others, without initiating booted providers etc again and so on.

Testing it right now, and will let you know how it goes...

saas786 commented 3 years ago
[22-Jun-2021 13:54:39 UTC] PHP Fatal error:  Uncaught Error: Class 'Theme\Lib\LoadingScreen' not found in theme\app\Provider.php:27
Stack trace:
#0 \hybrid-core\src\Container\Container.php(358): Exhale\Provider::Exhale\{closure}(Object(Hybrid\Core\Application), Array)
#1 \hybrid-core\src\Container\Container.php(177): Hybrid\Container\Container->build(Object(Closure), Array)
#2 theme\app\Provider.php(44): Hybrid\Container\Container->resolve('Theme\\Lib\\Loadin...')
#3 \hybrid-core\src\Core\Application.php(205): Exhale\Provider->boot()
#4 \hybrid-core\src\Core\Application.php(246): Hybrid\Core\Application->bootProvider(Object(Exhale\Provider))
#5 \hybrid-core\src\Core\Application.php(102): Hybrid\Core\Application->bootProviders()

even though it exists in bindings

[22-Jun-2021 13:54:39 UTC] Hybrid\Core\Application Object
(
    [providers:protected] => Array
        (

            [1] => Exhale\Provider Object
                (
                    [app:protected] => Hybrid\Core\Application Object

                )
        )

    [proxies:protected] => Array
        (
            [Hybrid\Proxies\App] => \Hybrid\App
        )

    [booted_providers:protected] => Array
        (

            [1] => Exhale\Provider Object
                (
                    [app:protected] => Hybrid\Core\Application Object

                )

        )

    [registered_proxies:protected] => Array
        (
            [0] => \Hybrid\App
        )

    [bindings:protected] => Array
        (

            [Theme\Lib\LoadingScreen] => Array
                (
                    [concrete] => Closure Object
                        (
                        )

                    [shared] => 1
                )

        )

    [aliases:protected] => Array
        (
            [template/manager] => Hybrid\Template\Manager
            [template/hierarchy] => Hybrid\Template\Contracts\Hierarchy
            [view] => Hybrid\View\Contracts\View
            [view/engine] => Hybrid\View\Contracts\Engine
            [attr] => Hybrid\Attr\Contracts\Attributes
        )

    [instances:protected] => Array
        (
            [app] => Hybrid\Core\Application Object

            [path] => \hybrid-core\src
            [version] => 6.0.0
            [Exhale\UI\Component] => Exhale\UI\Component Object
                (
                )

        )

    [extensions:protected] => Array
        (

            [Exhale\UI\Component] => Array
                (
                )

            [Theme\Lib\LoadingScreen] => Array
                (
                )
        )

)
justintadlock commented 3 years ago

I thought you were testing my Exhale theme at first, but then I saw some classes I didn't recognize. :)

Anyway, can you share the code? Just reading the error offhand, I don't see how it is related to this. A class not found error doesn't make sense here.

saas786 commented 3 years ago

I thought you were testing my Exhale theme at first, but then I saw some classes I didn't recognize. :)

Anyway, can you share the code? Just reading the error offhand, I don't see how it is related to this. A class not found error doesn't make sense here.

Yes, One of the theme used for this testing is based on Exhale, so that's why :) I will give it another go hopefully today and update you, and will share the code possible afterwards as well in case it still doesn't work.

justintadlock commented 3 years ago

I've been updating several packages and prepping Exhale to use the 6.0 work so I can do more thorough testing. I'm moving quick, so you might see quite a few changes.

saas786 commented 3 years ago

It seems to work, but not exactly how it should...

I can't debug the issue right now, but what I can see the issue could be:

Same as I described before, some package initiating the boot sequence of other package providers.

Which is not ideal...

Is it possible if we can group the providers / abstracts / proxies etc by some slug?

so when doing $slug->boot( 'unique-slug' );

This only boots this slug group of providers?

and also when resolving: App::resolve( 'some-thing', [], 'unique-slug' );

only resolves from the slug concrete / instance / abstract etc...

And when unique-slug is not provided, it just process default group...

??? Make sense...

justintadlock commented 3 years ago

I need code to test if something is or seems broken. I don't really have anything to go on other than successful tests on my end.

Same as I described before, some package initiating the boot sequence of other package providers.

The only providers that get booted are those that are registered at the time Application::bootProviders() runs. So, one package can't boot providers registered via a second package unless those providers are exactly the same (e.g., two packages using Hybrid\View).

saas786 commented 3 years ago

I need code to test if something is or seems broken. I don't really have anything to go on other than successful tests on my end.

I will see if I can provide you quick test setup, which replicates the issue, so you can take a look. Will try to do it over the weekend, otherwise over the week whenever I get enough time to take a dig on it.

saas786 commented 3 years ago

I have pulled all the latest code for framework and addons, and tested it again locally, it seems like its working without the above issue I faced.

Things look good.

Will do another couple of tests later this week, hopefully it will be all good :D.

Good work Justin.

justintadlock commented 3 years ago

Cool. I'll wait for any further feedback and leave this ticket open for now.

saas786 commented 2 years ago

I think we can close this now. As I have been using it for multiple packages, and so far had no issue.