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

[console] Do not override constructors for service objects, plugins, and controllers #4169

Closed
10 tasks done
GueGuerreiro opened this issue Oct 6, 2019 · 0 comments
Closed
10 tasks done

Comments

@GueGuerreiro
Copy link
Contributor

GueGuerreiro commented Oct 6, 2019

Issue title

Problem/Motivation

According to the Drupal 8 Backward Compatibility Policy, constructors for service objects are considered internal, which means they are subject to change on a minor release:

The constructor for a service object (one in the container), plugin, or controller is considered internal unless otherwise marked, and may change in a minor release. These are objects where developers are not expected to call the constructor directly in the first place. Constructors for value objects where a developer will be calling the constructor directly are excluded from this statement.

The implication is that if we override their constructors, and they happen to change their signature on a minor release, it would require us to update our current code for all instances where we're overriding their constructors, to conform to the new signature. This shouldn't have to happen on a minor release.

There is a good blog post from jrockwitz (maintainer of Webform for D8), that goes into more detail: https://www.jrockowitz.com/blog/webform-road-to-drupal-9

Solution

We can implement a new design pattern that would fix the problem.

As an example, from the generate:controller command, injecting 3 services as an example.

Before:

...
  public function __construct(
    RequestStack $request_stack,
    LoggerInterface $logger,
    Client $httpClient) {
    $this->request = $request_stack->getCurrentRequest();
    $this->logger = $logger;
    $this->httpClient = $httpClient;
  }

  public static function create(ContainerInterface $container) {
    return new static(
      $container->get('request_stack'),
      $container->get('logger.factory')->get('foo'),
      $container->get('http_client')
    );
  }
...

After:

...
  public static function create(ContainerInterface $container) {
    $instance = parent::create($container);
    $instance->request = $container->get('request_stack')->getCurrentRequest();
    $instance->logger = $container->get('logger.factory')->get('foo');
    $instance->httpClient = $container->get('http_client');
    return $instance;
  }
...

Some of the most used Drupal contrib modules are already using this design pattern, like the aforementioned Webform, Migrate Plus, Search API, etc.

Some extra benefits from this approach also include less code bloat:

  • We no longer need a __construct() method, since the __construct() and create() methods are effectively merged into one.
  • We no longer need a use statement per service injected. Their only purpose was so we could typehint on the constructor.

That reduces the final code lines by quite a bit. Also due to the above, adding a new service dependency injection after already running the generate command is no longer a major PITA.

I think this solution would include:

  • A change in drupal-console-core's TwigRenderer method to add a new twig function, similar to serviceClassInjection, that would use this new design pattern.
  • A change to the generator twig template files, to use this new function, and also remove the now unnecessary use statements as well as the constructor.

I have PR's ready for the new function on TwigRenderer and a change to the Controller generator to use it.

This solution could be applied to the following generators:

  • generate:controller
  • generate:entity:content
  • generate:form
  • generate:form:config
  • generate:plugin:block
  • generate:plugin:condition
  • generate:plugin:imageformatter
  • generate:plugin:mail
  • generate:plugin:rest:resource
  • generate:plugin:skeleton
enzolutions pushed a commit that referenced this issue Oct 11, 2019
* Issue  #4169 - generate:controller

* Issue #4169 - generate:form:config

* Issue #4169 - generate:form

* Smallfix.

Adds spacing between Twig `use_class_services` block declarations.

* Issue #4169 - generate:plugin:block

* Issue #4169 - generate:plugin:mail

* Issue #4169 - generate:plugin:skeleton

This one has a lot of coding standard errors,
as well as a malformed namespace.
I have not attempted to fix those, as they are
out of scope for this issue, although I fixed
them for the create() function, since that's
directly related to what I'm changing.

* Issue #4169 - generate:plugin:rest:resource

This one has some coding standard errors.
I have not attempted to fix those,
as they are out of scope for this issue, although I fixed
them for the create() function and the
currentUser property definition, since that's
directly related to what I'm changing.

* Issue #4169 - generate:plugin:condition

* Issue #4169 - generate:plugin:imageformatter

This one has some coding standard errors.
I have not attempted to fix those,
as they are out of scope for this issue, although I fixed
them for the create() function and the
property definitions, since that's
directly related to what I'm changing.

* Issue #4169 - generate:entity:content
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

No branches or pull requests

1 participant