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

Change the initialization from ServiceProviders and Subscribers #5630

Closed
CrochetFeve0251 opened this issue Dec 15, 2022 · 3 comments · May be fixed by #5732
Closed

Change the initialization from ServiceProviders and Subscribers #5630

CrochetFeve0251 opened this issue Dec 15, 2022 · 3 comments · May be fixed by #5732

Comments

@CrochetFeve0251
Copy link
Contributor

CrochetFeve0251 commented Dec 15, 2022

Is your feature request related to a problem? Please describe.
My solution aims to improve architecture from the initialization from ServiceProviders and Subscribers by removing the reference to subscribers inside the Plugin.php file.

Describe the solution you'd like
For that I would consider a solution using the same logic as rocket-cdn plugin to initialize the plugin.
However instead of one kind of subscriber referenced on service providers we would have 4 types:

  • Front subscribers
  • Admin subscribers
  • Common subscribers
  • License subscribers

Also as subscribers names will be hidden, we should create a prefix for each service provider that add before before the id from the class that would prevent conflict between two subscribers.

One drawback that I potentially saw is that we need to initialize every service provider to check for subscribers.
However as we don't use ServiceProviders constructors (we have no operations inside them) I don't think this would be a huge issue.

Another potential issue I saw was adding multiple time the same ServiceProvider to the container. However the container does a check to see if it is already there before adding it so that shouldn't be an issue.

Code

For that we could create a new base for all ServiceProvider that we will call AbstractServiceProvider with the following content:

use WP_Rocket\Dependencies\League\Container\ServiceProvider\AbstractServiceProvider as LeagueServiceProvider;


abstract class AbstractServiceProvider extends LeagueServiceProvider {
    protected $prefix = '';

   protected $front_subscribers = [];
   protected $admin_subscribers = [];
   protected $common_subscribers = [];
   protected $license_subscribers = [];

    public function provides(string $id): bool
    {
        return in_array($id, $this->provides) || in_array($id, $this->admin_subscribers) || in_array($id, $this->front_subscribers) || in_array($id, $this->common_subscribers) || in_array($id, $this->license_subscribers);
    }

  public function get_front_subscribers(): array {
    return $this->front_subscribers;
  }

  public function get_admin_subscribers(): array {
    return $this->admin_subscribers;
  }

  public function get_common_subscribers(): array {
    return $this->common_subscribers;
  }

  public function get_license_subscribers(): array {
    return $this->license_subscribers;
  }

   public function add(string $id, string $class) {
       $internal_id = $this->generate_container_id( $id );

        if( ! $this->provides( $internal_id ) && $expose ) {
            $this->provides[] = $internal_id;
        }
       return $this->getContainer()->add( $internal_id, $class); 
  }

 protected function getInternal(string $id)
    {
        return $this->getContainer()->get( $this->generate_container_id( $id ) );
    }

   protected function generate_container_id(string $id)
    {
        if ($this->prefix) {
            return $this->prefix . $id;
        }

        $class = get_class( $this );
        $class = dirname( $class );

        $class = trim( $class, '\\' );
        $class = str_replace( '\\', '.', $class );
        $class = strtolower( preg_replace( ['/([a-z])\d([A-Z])/', '/[^_]([A-Z][a-z])]/'], '$1_$2', $class ) );
        $this->prefix = $class . '.';

        return $this->prefix . $id;
    }
}

Concerning the Plugin.php we can change the logic to load subcribers like that:

 /**
     * Loads the plugin into WordPress.
     *
     * @since 3.0
     *
     * @return void
     */
    public function load() {
        $this->event_manager = new Event_Manager();
        $this->container->addShared( 'event_manager', $this->event_manager );

        $this->options_api = new Options( 'wp_rocket_' );
	$this->container->add( 'options_api', $this->options_api );
	$this->container->addServiceProvider( OptionsServiceProvider::class );
	$this->options = $this->container->get( 'options' );

        $this->load_subscribers();
    }

    /**
     * Get the subscribers to add to the event manager.
     *
     * @since 3.6
     *
     * @return array array of subscribers.
     */
    private function load_subscribers() {
        $providers = [
            new AdminDatabaseServiceProvider(),
            new SupportServiceProvider(),
            new BeaconServiceProvider(),
            new RocketCDNServiceProvider(),
            new CacheServiceProvider(),
            new CriticalPathServiceProvider(),
            new HealthCheckServiceProvider(),
            new MediaServiceProvider(),
            new DeferJSServiceProvider(),
            new SettingsServiceProvider(),
            new EngineAdminServiceProvider(),
            new OptimizationAdminServiceProvider(),
            new CapabilitiesServiceProvider(),
            new AddonServiceProvider(),
            new VarnishServiceProvider(),
            new PreloadServiceProvider(),
            new PreloadLinksServiceProvider(),
            new CDNServiceProvider(),
            new Common_Subscribers(),
            new ThirdPartyServiceProvider(),
            new HostingsServiceProvider(),
            new PluginServiceProvider(),
            new DelayJSServiceProvider(),
            new RUCSSServiceProvider(),
            new HeartbeatServiceProvider(),
            new DynamicListsServiceProvider(),
            new LicenseServiceProvider(),
            new ThemesServiceProvider(),
            new APIServiceProvider(),
        ];

        if ( is_admin() ) {
            $this->init_admin_subscribers( $providers );
        } elseif ($this->is_valid_key ) {
            $this->init_valid_key_subscribers( $providers );
        }

       if ( ! is_admin() ) {
        $this->init_front_subscribers();
      }

        $this->init_common_subscribers( $providers );
    }

    /**
     * Initializes the admin subscribers.
     *
     * @since 3.6
     *
     * @return array array of subscribers.
     */
    private function init_admin_subscribers(array $providers) {
        $this->init_subscribers( $providers, 'get_admin_subscribers');
    }

    private function init_valid_key_subscribers(array $providers) {
        $this->init_subscribers( $providers, 'get_license_subscribers');
    }

    private function init_common_subscribers(array $providers) {
        $this->init_subscribers( $providers, 'get_common_subscribers');
    }


    private function init_front_subscribers(array $providers) {
        $this->init_subscribers( $providers, 'get_front_subscribers');
    }


    /**
     * Initializes the admin subscribers.
     *
     * @since 3.6
     *
     * @return array array of subscribers.
     */
    private function init_subscribers(array $providers, string $method) {
        /** @var AbstractServiceProvider[] $providers */
        foreach ($providers as $provider) {
            if(! $provider->$method() ) {
              continue;
            }
            $this->container->addServiceProvider( $provider );
             $this->add_subscribers( $provider->$method() );
        }
    }

   protected function add_subscribers(array $subscribers) {
      foreach ( $subscribers as $subscriber ) {
			$this->event_manager->add_subscriber( $this->container->get( $subscriber ) );
		} 
    }
@piotrbak
Copy link
Contributor

@MathieuLamiot What do you think about this one?

@MathieuLamiot
Copy link
Contributor

To keep opened for now. We'll probably move it to WP Launchpad though. I'll make sure it's handled next week.

@CrochetFeve0251
Copy link
Contributor Author

@MathieuLamiot I am not sure if this needs to be closed as it will be needed to then make wpr compatible with launchpad.

However, I made a like to that issue on Launchpad: wp-launchpad/launchpad#25

@MathieuLamiot MathieuLamiot closed this as not planned Won't fix, can't repro, duplicate, stale Jul 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants