In #2392319: Config objects (but not config entities) should by default be immutable we need to be able to use a trait in a form alter. This is so that form alter can save configuration once \Drupal::config() returns immutable config. For discussions on why this is preferable see that issue.

This change also has the advantage of making it easy to use new D8 best practices for form alters. For example, constructor injection and only autoloading the minimum amount of code to perform a request. Plus form alters are one of the entry points to drupal it would be great if they looked more like the rest of D8.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Task because work arounds exist.
Issue priority Major because form alters are one of the entry points to drupal and modifying configuration forms to add site specific options can be common on custom sites. It is super easy to get the config override wrong and therefore end up with overrides being stored in your configuration.
Disruption Not disruptive - contrib is free to continue to implement form alters in a procedural fashion.
Files: 
CommentFileSizeAuthor
#15 2402445-oop-form_alter-15.patch30.98 KBvijaycs85
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,082 pass(es), 81 fail(s), and 2 exception(s). View

Comments

alexpott’s picture

Issue summary: View changes
Priority: Normal » Major
Status: Active » Needs review
Issue tags: -blocker
Parent issue: » #2392319: Config objects (but not config entities) should by default be immutable
FileSize
30.74 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,495 pass(es). View

I can't decide if this is critical since this could block #2392319: Config objects (but not config entities) should by default be immutable. But actually I think I want progress on that issue concurrently with this one - so I'll fix the alters in a procedural but less nice way on that issue and depending on which one gets in first - either issue can implement using the trait on the alter objects.

dawehner’s picture

+++ b/core/modules/contact/contact.module
@@ -147,33 +147,10 @@ function contact_mail($key, &$message, $params) {
+  /** @var \Drupal\contact\Form\UserFormAlter $alter */
+  $alter = \Drupal::resolve('Drupal\contact\Form\UserFormAlter');
+  $alter->alter($form, $form_state);
+  $form['actions']['submit']['#submit'][] = array($alter, 'submit');
 }

@@ -181,32 +158,12 @@ function contact_user_profile_form_submit($form, FormStateInterface $form_state)
 function contact_form_user_admin_settings_alter(&$form, FormStateInterface $form_state) {
...
+  /** @var \Drupal\contact\Form\UserAccountSettingsFormAlter $alter */
+  $alter = \Drupal::resolve('Drupal\contact\Form\UserAccountSettingsFormAlter');
+  $alter->alter($form, $form_state);
   // Add submit handler to save contact configuration.
...
+  $form['#submit'][] = array($alter, 'submit');
 }

Why is the $form['#submit][] call not inside the alter code itself? I don't see a reason for that.

larowlan’s picture

Why not #2237831: [PP-2] Allow module services to specify hooks - allowing all hooks to be OO? Seems counter intuitive to DX to limit it to just one hook.

alexpott’s picture

@larowlan well this does not make the hook OO - it just uses the procedural hook as a bridge to an object. I'm happy to admit I'm not a huge fan. But this was the smallest possible change. On the subject of OO hooks we've also had #1972304: Add a HookEvent. The thing that bothers me about #2237831: [PP-2] Allow module services to specify hooks is the HOOK class would contain many form alters- which seems to break the idea of encapsulating related logic.

larowlan’s picture

The thing that bothers me about #2237831: Allow module hooks to reside in a Drupal\$module\Hook class is the HOOK class would contain many form alters- which seems to break the idea of encapsulating related logic

yes agree

tim.plunkett’s picture

There was also #2328871: Remove FormBuilder coupling to moduleHandler/themeManager, which had one approach that specifically added events for hook_form_alter.

effulgentsia’s picture

which had one approach

The key word there is had, past tense, because that issue moved away from that. But that issue still has a FormAlterInterface, so adding it as a related issue.

Seems counter intuitive to DX to limit it to just one hook.

Adding #1972304: Add a HookEvent as a related issue, but the purpose of this issue is different than that one. The key goal of this issue is to have the alter's alter() and submit() be on the same class, so that they can share a protected member, just as we have for FormInterface's buildForm() and submitForm(). If #1972304: Add a HookEvent were done, then we could achieve that by putting submit() into the corresponding subscriber, but I don't think it makes sense to delay this issue on that one.

Why is the $form['#submit][] call not inside the alter code itself? I don't see a reason for that.

See also #2392319-29: Config objects (but not config entities) should by default be immutable.4, where I left that as a @todo, because I didn't have a clear preference either. On the one hand, for FormInterface, buildForm() is not responsible for wiring up validateForm() and submitForm(): that happens from the outside. On the other hand, I don't know that the procedural wrapper really qualifies as a good "outside", so maybe better to put the responsibility into the alter() unless/until we have a more proper "outside" for it.

+++ b/core/lib/Drupal.php
@@ -628,4 +628,20 @@ public static function accessManager() {
+  public static function resolve($definition) {

Maybe not the best name? See #2392319-50: Config objects (but not config entities) should by default be immutable. Anyone have other suggestions?

+++ b/core/lib/Drupal/Core/Form/FormAlterInterface.php
@@ -0,0 +1,53 @@
+  public function alter(array &$form, FormStateInterface $form_state);
+  public function validate(array &$form, FormStateInterface $form_state);
+  public function submit(array &$form, FormStateInterface $form_state);

What do we think of these names? Should we rename to validateForm() and submitForm() for consistency with FormInterface, or is the lack of consistency better, since this is an additional validate()/submit() rather than the form's "primary" one?

dawehner’s picture

In general I would ask myself whether validate() and submit() really belongs into the interface (given that validate/submit is often not part of the altering)
but rather should be added just in case someone needs it.

alexpott’s picture

I've look at all the suggested solutions for OO-ifying the hook system (#1972304: Add a HookEvent, #2237831: [PP-2] Allow module services to specify hooks and #2043653: Allow modules to implement hooks by nominating methods in a module.implements.yml) and I've concluded that there is an surmountable barrier to implementing any of them in D8. For me it comes down to the fact that the one hook implementation per module limitation of the current system and service architecture for objects is incompatible.

The compromise this issue makes using procedural bridges is something contrib will be able to do anyway and having an interface for this is nice.

re #8 - how about an interface per method? FormAlterInterface, FormAlterSubmitInterface and FormAlterValidateInterface?

re #2 - i did that because that was @effulgentsia's original suggestion - i think you are right.

alexpott’s picture

FileSize
14.68 KB
31.09 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,425 pass(es). View

The patch attached implements new interface for form alters that submit or validate. Moves the adding of the submit inside the alter methods. Removes some incorrect use statements.

effulgentsia’s picture

  1. +++ b/core/lib/Drupal.php
    @@ -628,4 +628,20 @@ public static function accessManager() {
    +  /**
    +   * Returns a class instance with a given class definition.
    +   *
    +   * @param string $definition
    +   *   A class name or service name.
    +   *
    +   * @throws \InvalidArgumentException
    +   *   If $class is not a valid service identifier and the class does not exist.
    +   *
    +   * @return object
    +   *   The instance of the class.
    +   */
    +  public static function resolve($definition) {
    +    return static::$container->get('class_resolver')->getInstanceFromDefinition($definition);
    +  }
    +
    

    Drupal:: already has a service() method, so how about renaming this from resolve() to createInstance() and making the argument $class instead of $definition? And having a class_exists() check to enforce that?

  2. +++ b/core/lib/Drupal/Core/Form/FormAlterInterface.php
    @@ -0,0 +1,31 @@
    + *   $alter = \Drupal::resolve('CLASSNAME');
    + *   $alter->alter($form, $form_state);
    

    How about a 1 line pattern: \Drupal::createInstance('CLASSNAME')->alter($form, $form_state)?

  3. +++ b/core/lib/Drupal/Core/Form/FormAlterSubmitInterface.php
    @@ -0,0 +1,27 @@
    +interface FormAlterSubmitInterface extends FormAlterInterface {
    +interface FormAlterValidateInterface extends FormAlterInterface {
    

    How about creating a FormSubmitInterface and FormValidateInterface that don't extend FormAlterInterface, and only have the submitForm() and validateForm() methods, make FormInterface extend both of those, and alters with validation/submission implement both FormAlterInterface and those? Potential downside of that: Drupal\Core\Form already has FormSubmitterInterface and FormValidatorInterface, so would those name suggestions be too similar and create confusion?

alexpott’s picture

FileSize
5.61 KB
31.22 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 83,935 pass(es). View
  1. Done - although the class_exists is a shame because we are repeating ourselves - the class_exists check is also done in the class_resolver service
  2. Done
  3. For me submitForm & validateForm imply they are acting on the whole form - which is unlikely for a form alter. That is why I prefer validate and submit. Also the fact they operate on forms is indicated by the word Form in the interface name.

alexpott queued 12: 2402445.12.patch for re-testing.

Crell’s picture

+++ b/core/modules/syslog/syslog.module
@@ -33,61 +32,7 @@ function syslog_help($route_name, RouteMatchInterface $route_match) {
+  /** @var \Drupal\syslog\Form\SystemLoggingSettingsFormAlter $alter */
+  $alter = \Drupal::resolve('Drupal\syslog\Form\SystemLoggingSettingsFormAlter');
+  $alter->alter($form, $form_state);

As long as you still need to do this dance yourself, I'm not seeing any benefit, honestly. Can we fold this into FAPI somehow so that you don't need the hook at all? *That* would be a good thing to do.

vijaycs85’s picture

FileSize
30.98 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 90,082 pass(es), 81 fail(s), and 2 exception(s). View

Rerolling #12

Status: Needs review » Needs work

The last submitted patch, 15: 2402445-oop-form_alter-15.patch, failed testing.

xjm’s picture

Version: 8.0.x-dev » 9.x-dev
Status: Needs work » Postponed

@alexpott gave an example of the issue we are trying to solve here:
syslog_form_system_logging_settings_alter()

We discussed that the specific solution described in this issue title is likely too big a change during the beta, even with BC. A better workaround for 8.0.x might be to provide (e.g.) a FAPI key that can contain the IDs of any additional config objects added to the form, and then we can check that list on submit. @alexpott will file a new major bug for that.

Postponing to 9.x, though there are probably a few other issues out there for this as well.

We discussed that we need to do #2408549: There is no indication on configuration forms if there are overridden values still, though, @Gábor Hojtsy had some concerns about that issue not accomplishing the full goal.

xjm’s picture

dawehner’s picture

Here is a question. I do agree that for 8.0 its too late. On the other hand the entire concept of minor version releases in 8.x
will include new features, so would make it possible to have OOP alters be a feature possible for 8.1 or 8.2 for example?

Crell’s picture

dawehner: In concept, possibly, provided they could be done in an entirely BC way and we decide that it's a worthwhile path to go down. Given that FAPI is probably due for a heavy overhaul in Drupal 9 anyway, it may or may not be worth it to invest time in attaching more OOP behaviors to it now. (Really, I don't know.)

tim.plunkett’s picture

Version: 9.x-dev » 8.1.x-dev

I'm very confident that we can do this in a safe BC way in 8.1.x

xjm’s picture

BTW @tim.plunkett pinged me about doing this in 8.1.x with BC and I agreed that makes sense.

joelpittet’s picture

Status: Postponed » Needs work

Unpostponing as the branch is active

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.