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

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

Rerolled #21, addressed #19 and #26, and updated to the latest core standards.

@smustgrave those new services are event subscribers, so they're not APIs that need to be announced in the CR :)

The last submitted patch, 28: 3208390-28.patch, failed testing. View results →

Fixed the test failure.

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.

amateescu → changed the visibility of the branch 3208390-add-an-api to hidden.

amateescu → changed the visibility of the branch 11.x to hidden.

Since @catch was not a fan of the event approach, here's an alternative that would allow modules to declare their forms as workspace-safe even when the workspaces module is not installed.

I added the trait in the Drupal\Core\Form as an initial proposal, but it could also be added somewhere in the system module I guess..

Once Workspaces will be marked as stable, more and more modules will have to integrate with it, so I'm raising the priority of this issue.

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.

Hiding old patch files.

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.

@alexpott the idea with a trait came up specifically for the dynamic nature that some modules might need, like Layout Builder in core :)

The difference is that in HEAD, LB has to call a method in each form class for marking its forms as workspace-safe, while with the trait approach it only has to ensure that its method name matches the one from the "top-level" trait, and Workspaces takes care of calling it in its own form alter.

I don't mind going for the interface approach you suggested, Workspaces itself does that currently and it works great when there's no dynamism involved. Should we wait or ask for other opinions or do you feel (strongly or not) that the interface will be far simpler to implement for the majority use cases?

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.

Changed the approach to use two interfaces instead of a trait, and updated the issue summary and the change record.

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!

Automatically closed - issue fixed for 2 weeks with no activity.