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

Better CLI loading for ServiceInterface implementations #438

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

mbmjertan
Copy link
Collaborator

@mbmjertan mbmjertan commented Oct 17, 2024

Description

Currently, if you want to load a ServiceInterface-implementing class in the WP-CLI context, such as a class inheriting from AbstractPostType or AbstractTaxonomy, you're out of luck. And you really should load those in the CLI context.

Only classes implementing ServiceCliInterface are loaded in the CLI context, and they're aren't loaded in the web context. Only classes implementing ServiceInterface are loaded in the web context, and you've guessed it.

This could be worked around by just making no such distinction, but that lowers developer flexibility and makes you unable not to load e.g. CLI commands in the web context.

Instead of such compromises, this PR proposes an alternative way to indicate a ServiceInterface should be loaded in CLI contexts using a class attribute, a cool new PHP 8 feature, alongside our good and trusted friend ReflectionClass. We have been using this without any performance penalty on the web and nothing noticeable in WP-CLI for months now.

If this PR is merged, you'll be able to ensure a service class gets loaded in the WP-CLI context like so:

<?php
namespace EightshiftProject\Services;

#[ShouldLoadInCliContext]
class SomeServiceClass implements ServiceInterface 
{
  // ...
}

Additionally, if your class is a child of any class annotated with the ShouldLoadInCliContext attribute, it will be loaded, so you can annotate AbstractTaxonomy with the mark of the devil attribute or do things like these:

<?php
namespace EightshiftProject\Things;

#[ShouldLoadInCliContext]
abstract AbstractSomethingService implements ServiceInterface
{
  // ...
}
<?php
namespace EighshiftProject\Things;

class SomethingService extends AbstractSomethingService {
  // ...
}

I believe this is a valuable backend addition to eightshift-libs and it has helped us to entirely eliminate issues with loading post types, taxonomies and other shared service interfaces in our projects. I am open to any and all comments.

If this gets merged, please also mark AbstractPostType and AbstractTaxonomy with the attribute.

This commit adds a new class attribute that can be used to indicate a service class (i.e. implementing ServiceInterface) should be loaded in the CLI context
This commit adds a new class attribute that can be used to indicate a service class (i.e. implementing ServiceInterface) should be loaded in the CLI context.
Changes registerServices() so it only additionally loads ShouldLoadInCliContext annotated classes if and only if under WP-CLI.
@mbmjertan mbmjertan added improvement Small improvement fixes, either readability or performance improvements wpcli Issues regarding WP-CLI labels Oct 17, 2024
@mbmjertan mbmjertan requested review from dingo-d and a team October 17, 2024 18:38
@mbmjertan mbmjertan self-assigned this Oct 17, 2024
Copy link
Collaborator

@dingo-d dingo-d left a comment

Choose a reason for hiding this comment

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

👍🏼 LGTM

Copy link
Contributor

@piqusy piqusy left a comment

Choose a reason for hiding this comment

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

Nice

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement Small improvement fixes, either readability or performance improvements wpcli Issues regarding WP-CLI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants