Problem/Motivation

Add object that uses the FactoryInterface should have dependencies inject via ::create and not ::__construct(). This allows parent classes to change their constructor without breaking any code.
See https://events.drupal.org/seattle2019/sessions/drupal-9-coming-getting-y...
See https://www.drupal.org/core/d8-bc-policy

Example: \Drupal\config_sync\Controller\ConfigSyncController::create
See https://www.youtube.com/watch?time_continue=1554&v=hN9KjaBvAUk
See https://www.previousnext.com.au/blog/safely-extending-drupal-8-plugin-cl...

Proposed resolution

Fix subclassing and stop overriding constructors.

CommentFileSizeAuthor
#7 Captura de tela de 2020-04-29 09-15-15.png158.43 KBilgnerfagundes
#2 xmlsitemap-Fix_subclassing_and_stop_overriding_constructors_in_xmlsitemap_Form_XmlSitemapSettingsForm-3121750-2-D8.patch2.16 KBthalles

Comments

thalles created an issue. See original summary.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Status: Needs work » Needs review
StatusFileSize
new xmlsitemap-Fix_subclassing_and_stop_overriding_constructors_in_xmlsitemap_Form_XmlSitemapSettingsForm-3121750-2-D8.patch2.16 KB

Follow a patch

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Needs review » Postponed (maintainer needs more info)

ConfigFormBase isn't just any old class though, it's intended use it to be extended. Is there an issue where this is being discussed or fixed in core?

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

The other issue with doing this is when plugin base classes have logic inside their __construct() methods, such as calling setConfiguration(). If in our own plugin's defaultConfiguration() method we rely on an additional service, then it will be undefined when the parent's constructor is called.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented

The documentation class apparently asked to the denpendency injection will be make this:

Drupal\Core\Form\FormBase

/**
 * Provides a base class for forms.
 *
 * This class exists as a mid-point between dependency injection through
 * ContainerInjectionInterface, and a less-structured use of traits which
 * default to using the \Drupal accessor for service discovery.
 *
 * To properly inject services, override create() and use the setters provided
 * by the traits to inject the needed services.
 *
 * @code
 * public static function create($container) {
 *   $form = new static();
 *   // In this example we only need string translation so we use the
 *   // setStringTranslation() method provided by StringTranslationTrait.
 *   $form->setStringTranslation($container->get('string_translation'));
 *   return $form;
 * }
 * @endcode
 *
 * Alternately, do not use FormBase. A class can implement FormInterface, use
 * the traits it needs, and inject services from the container as required.
 *
 * @ingroup form_api
 *
 * @see \Drupal\Core\DependencyInjection\ContainerInjectionInterface
 */
thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented
Status: Postponed (maintainer needs more info) » Needs review
ilgnerfagundes’s picture

ilgnerfagundes commented
Status: Needs review » Reviewed & tested by the community
StatusFileSize
new Captura de tela de 2020-04-29 09-15-15.png158.43 KB

The patch was applied correctly, and the form is working perfectly.

neclimdul’s picture

neclimdul
He/Him
commented
Status: Reviewed & tested by the community » Postponed (maintainer needs more info)

Yeah, I'm not buying that. Is there some core discussion around this I missed? This is just silly and dead wrong.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented

https://events.drupal.org/seattle2019/sessions/drupal-9-coming-getting-y...
https://youtu.be/hN9KjaBvAUk?t=1443

neclimdul’s picture

neclimdul
He/Him
commented

I saw those in your summary but a talk at Drupalcon isn't a framework decision. While valid this is _not_ how the API was designed to be used. I'll talk to Alex I guess.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented

Is this FormBase documentation not valid?

neclimdul’s picture

neclimdul
He/Him
commented

I strongly believe it is wrong yes. Or at the least very misleading.

This class exists as a mid-point between dependency injection through ContainerInjectionInterface, and a less-structured use of traits

The misleading part is "less structured use of traits" and StringTranslationTrait::setStringTranslation. It is not obvious that saying to populate the trait dependency(which doesn't have a constructor because its a trait) and not bypassing injecting dependencies into the constructor. Should that also be in the constructor? Maybe. Its a lot different form moving all constructor logic into the create method though.

I had a pretty long discussion on slack last night and this morning with some core developers and I'm not sure there was a consensus in anything other than "inheritance got us to this mess and is generally pretty terrible". I'll open the core issue to resolve/clarify the documentation on form base. We can capture and continue the discussion there.

thalles’s picture

thalles
Primary language Portuguese, Brazil
Location Teófilo Otoni - MG
commented

Ok!
When you create, shared the link here, please!

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

@neclimdul: Any chance you have filed that core issue?