Add an API for allowing modules to mark their forms as workspace-safe

Overall looks great, and the event will work really well - I think - Code is really clean.

There are normally 3 cases, but one is not really needed:

- Form is safe to submit while in a workspace and in Live [workspace_safe = TRUE]
(mostly GET request forms -- maybe we could mark those automatically as safe as they must not mutate data per HTTP specification?)

- Form is not safe to submit while in a workspace, but safe to submit in live [workspace_safe == FALSE], e.g. config entities

- Form is safe to submit, while in a workspace, but not in Live => We don't need this case

==> The boolean is fine to use (and pre-existing anyway).

While a form alter is as centralized (because the event piggy-backs on the form alter), I can see that the event is making the logic cleaner.

I defer to core committers to decide if that's a warranted use-case for a new event.

Unrelated, but saw this here:

- We could consider a follow-up to automatically mark all GET request forms as safe. If someone violates HTTP spec, I don't think that is a problem of Core to protect them.

- Overall I think contrib will provide a way to mark all forms as safe -- if you have a special permission, but I am not sure if that's a core thing.

Because I can see the case of a developer that wants to develop views while in a workspace. [e.g. being really frustrated of that safeness feature]

Nice patch!

+++ b/core/modules/workspaces/src/EventSubscriber/DefaultWorkspaceSafeForms.php
@@ -0,0 +1,42 @@
+    $form_object = $event->getFormState()->getFormObject();
+    $is_workspace_form = $form_object instanceof WorkspaceFormInterface;
+    $is_views_exposed_form = $form_object instanceof ViewsExposedForm;
+    $is_search_form = in_array($event->getFormId(), ['search_block_form', 'search_form'], TRUE);
+

Wouldn't search and views module be responsible for implementing this? Since it's core it doesn't really matter and workspaces could do it on their behalf, but more to the point there are no implementation examples outside of workspaces module itself.

Also needs a change record and tests.

I'm not keen on adding events to core, but that's a separate issue to the need for an API for this.

Even though it just means another two subscribers, I'm +1 to doing things "correctly" and having more examples of how it's done. One for search, one for views.

MR is empty.

I will provide patches instead of using an MR as discussed with amateescu because we need a packport to 8.x , so patches seem to be easier

Here's first patch and interdiff adding tests as requested. I didn't add a dedicated test method to test workspaces internal forms as that's already being done within the added test methods when the workspace gets switched, as the switcher form is one of these module internal forms.

I can still add tests for more internal forms if that's desired. Also, I'll provide a backport of the patch for 8.x sometime soon, will set this to on needs review once I got the backport ready too.

Here's the patch for 8.9.x .

> - it scatters the knowledge about workspace integration across many files (forms), instead of a centralized place

From the point of view of the module that defines the form, that's a good thing though, isn't it? The form class defines the form, and having the form class also declare itself as workspace-safe keeps things about the form in the same place.

The current MR is requiring modules that define forms to add a lot of boilerplate, and an event seems like overkill for something that sounds like it's declarative.

I can see that having to do '$form_state->set()' means that you can't know whether a form is OK until you've actually created the form build array.

But what about adding a method to the form class? The one drawback I can see with that is if a hook_form_alter() changes the workspace-safety of a form. But with the event, the module doing the form alteration has to be pretty careful with event weights to make sure it fires after the form-defining module's event! To cover that case, we could keep the $form_state->set() for dynamic cases?

Having looked over this patch, I have a couple thoughts about the EventSubscribing portions of this that I hope will bring us more in line with where Symfony is going, but I'm unsure of the current approach within core to this space.

1. +++ b/core/modules/workspaces/src/FormOperations.php
   @@ -71,20 +82,9 @@ public function formAlter(array &$form, FormStateInterface $form_state, $form_id
   +    $this->dispatcher->dispatch($event, WorkspaceEvents::WORKSPACE_SAFE_FORM);
   

   Are we at a point where we can safely stop assigning names to the event and just use the event class convention in Symfony?

2. +++ b/core/modules/workspaces/src/WorkspaceEvents.php
   @@ -0,0 +1,18 @@
   +<?php
   

   If we can stop naming events, we could remove this class. Even if we can't stop doing that yet, we could potentially use WorkspaceSafeFormEvent::class as the name. That would conform to the new standard, and allow us to just drop the second parameter when the time comes.

Not sure #19 is in scope; followup maybe?

I have no meaningful feedback on the API addition itself, but since this has been stuck in the RTBC queue for six months, here is your "committer guilt review". :)

1. This is apparently missing some docs which is causing PHPCS fails. Not sure why the issue didn't get NWed; probably the bulk update.
    
2. Was #7 addressed? Edit: Maybe it didn't need to be because the patch already does that? I think it is reasonable as a response to @catch's feedback in #6.
    
3. There are at least a few places that could also have return typehints added, especially where it is pure API addition (rather than just subclassing). Lots of missing void return typehints in particular.

4. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceSafeFormTest.php
   @@ -0,0 +1,147 @@
   +  protected static $modules = ['search', 'views', 'workspaces', 'node', 'block'];
   

   This is 81 chars and so should use multiline array syntax.

5. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceSafeFormTest.php
   @@ -0,0 +1,147 @@
   +   * @param array $permissions
   +   *   Additional permissions to be granted to the created user.
   

   string[]?

6. +++ b/core/modules/search/src/EventSubscriber/SearchWorkspaceSafeFormsSubscriber.php
   @@ -0,0 +1,38 @@
   +   * Marks search forms as workspace safe.
   

   Nit (here and elsewhere): "workspace-safe" should be hyphenated.

7. +++ b/core/modules/workspaces/src/Event/WorkspaceSafeFormEvent.php
   @@ -0,0 +1,102 @@
   +   * Gets the form id.
   

   Nit: "ID" should be uppercase.

8. +++ b/core/modules/workspaces/tests/src/Functional/WorkspaceSafeFormTest.php
   @@ -0,0 +1,147 @@
   +    $vultures = $this->createWorkspaceThroughUi('Vultures', 'vultures');
   +    $gravity = $this->createWorkspaceThroughUi('Gravity', 'gravity');
   +
   +    return [$test_node, $test_node2, $vultures, $gravity];
   

   Excellent test data nouns.

9. +++ b/core/modules/workspaces/src/FormOperations.php
   @@ -24,14 +24,24 @@ class FormOperations implements ContainerInjectionInterface {
   -  public function __construct(WorkspaceManagerInterface $workspace_manager) {
   +  public function __construct(WorkspaceManagerInterface $workspace_manager, EventDispatcherInterface $dispatcher) {
        $this->workspaceManager = $workspace_manager;
   +    $this->dispatcher = $dispatcher;
      }
   

   Almost gave kneejerk feedback that this needed a deprecation for the new service, but that is not necessary since the class has a factory method to inject the services.

   Edit: Fixing bad code snippet. Dreditor and GitLab require opposite cursor-drag directions to highlight lines in the diff and apparently I'm starting to adapt to GitLab despite more than a decade of core patches. :P

NW for points 1-7. Thanks!

This is working fine and I guess it's time to set it to RTBC once someone else confirms. I guess it's also worth mentioning that there's some other approach to this topic in the workspaces extra module's issue over here: #3304460: Different handling of form submissions inside workspaces

@s_leu, the next step is indeed a code review of @amateescu's fixes.

Issue summary as a TBD for a release note, that probably still needs to happen.

Think the change record should include the new service
+ search.workspace_safe_forms:
+ class: Drupal\search\EventSubscriber\SearchWorkspaceSafeFormsSubscriber
+ tags:
+ - { name: event_subscriber }

getSubscribedEvents
Should be typehinted

Change record also doesn't mean new event/service for views ViewsWorkspaceSafeFormsSubscriber

Change record reads well but could it or the issue summary be updated that this change is also marking certain core blocks as workspace safe? The change makes sense 100% just feels like something that should be noted.

Adding a patch that still applies on 10.1.x as the latest patch here doesn't any more.

The Needs Review Queue Bot tested this issue.

While you are making the above changes, we recommend that you convert this patch to a merge request. Merge requests are preferred over patches. Be sure to hide the old patch files as well. (Converting an issue to a merge request without other contributions to the issue will not receive credit.)

RTBC

My only concern was for existing code that sets workspace_safe = TRUE or FALSE, but the $form_state property always takes precedence, so this is just a way to set the default state of the form.

And it's very nice to use.

Why a trait over an empty interface in the Drupal\Core\Form namespace? There no logic or possible dynamism with an interface (or is that an intended side effect... sometimes a form is workspace safe and sometimes it is not... that feels odd).

I think

$form_state->set('workspace_safe', $form_state->getFormObject() instanceof Drupal\Core\Form\WorkspaceSafeInterface);

is nicer than messing with a trait and possible dynamic safeness. Anything that wants to be dynamic can still manipulate form state and set workspace_safe as they see fit.

Sorry I completely missed that we already have the dynamic use-case in core - so the trait implementing a method works out nicely there I agree.

What's not so nice is

    if (method_exists($form_state->getFormObject(), 'isWorkspaceSafeForm')) {
      $workspace_safe = $form_state->getFormObject()->isWorkspaceSafeForm($form, $form_state);
    }
    else {
      // No forms are safe to submit in a workspace by default.
      $workspace_safe = FALSE;
    }

we're making assumptions about the signature of isWorkspaceSafeForm that we can't really. So... I think we should add an interface WorkspaceSafeInterface that declares one method - isWorkSpaceSafeForm() and have the two trait be implementations of the interface for people to use.

    if ($form_state->getFormObject() instanceof WorkspaceSafeInterface)) {
      $workspace_safe = $form_state->getFormObject()->isWorkspaceSafeForm($form, $form_state);
    }
    else {
      // No forms are safe to submit in a workspace by default.
      $workspace_safe = FALSE;
    }

Then at least we are programming to interfaces and people can come up with their own implementations safe in the knowledge that they are implementing an interface.

Yes adding an interface and a trait is not as nice as just adding a trait - and maybe PHP will solve that someday but I think it is a price worth paying in order to avoid calling a method incorrectly and ding type-safe programming.

Back to RTBC!

The new approach is fully BC compatible for layout_builder and makes it much easier for forms to mark themselves as safe by default / definition.

This looks great now, glad I pushed back on the new event, much easier to see what's going on everywhere.

Committed/pushed to 11.x and cherry-picked to 10.3.x, thanks!