Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at

I think the default #action should be a placeholder (e.g. if no-one else set the action) that is always POSTing to the currently active URL, but because that means the form should have no state in itself, this is just possible when caching forms on GET requests is gone ... (#2502785: Remove support for $form_state->setCached() for GET requests)

Making forms cacheable just per page is ... problematic.

Nice find!

Thk ! Good to know.

Hey Everyone,

There are two scenarios to this.

1. When the form is doing drupal_set_message its going to system/404.
2. When the form is showing an error it goes to the first URL form was viewed.

Attached a patch for testing if required.

Thanks

Special thanks to @valthebald for helping me with this issue.

Attached the revised patch. Last one was not complete.

Thanks

@fabianx: my patch is only for testing reproducing this issue and it doesn't solves the issue.

Thanks

Hi, thanks for sharing that, had a lot of trouble with that issue tho.

Setting this back to needs work because the patch doesn't solve the issue, but only demonstrates the problem.

Let's change the patch to be an actual failing test so we also have a regression test when the issue is solved.

Added a new test that fails because of this.

Cleans up the code in the test and improves the test, nothing done to make the test pass yes though, so it doesn't.

So the bit of code that needs to be updated here is

    // Only update the action if it is not already set.
    if (!isset($form['#action'])) {
      $form['#action'] = $this->buildFormAction();
    }

There are two possible ways to solve this:

1. The simple-but-bad-for-cacheability way: just associate the url cache context with $form. I.e. $form['#cache']['contexts'][] = 'url'; (Try that, that should make the tests pass.)
2. The more-complex-but-good-for-cacheability way: Set $form['#action to a placeholder, and attach this placeholder. I.e. $form['#attached']['placeholders'][] = … Look at \Drupal\Core\Access\RouteProcessorCsrf::processOutbound() (#2512132: Make CSRF links cacheable) for an example.

Attached patch passes the test in the more-complex-but-good-for-cacheability way. I tested the easier way and that worked as well.

It needs more work, because right now we're still route_processor_csrf:renderPlaceholderCsrfToken in the #lazy_builder. I just wanted to make sure I was going in the right direction before continuing work on this.

We're now using a method in the FormBuilder class as #lazy_builder callback, so this is getting closer.

Locally this fixed tests for BlockContentTranslationUITest, let's see what other tests this fixes.

That's starting to look like something :)

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -613,7 +622,15 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      $form_action = $this->buildFormAction();
   +      $placeholder = hash('sha1', $form_action);
   

   This doesn't make sense; the placeholder is based on the already-built form action which is simply thrown away.

   It can be simplified to something like

   $placeholder = hash('sha1', $form_id)
   

   (It just needs to be a not-easily-guessable string.)

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -613,7 +622,15 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +        '#lazy_builder' => ['form_builder:renderPlaceholderFormAction', [$form_action]],
   

   This [$form_action] bit is wrong; the lazy builder callback doesn't need/use that.

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -613,7 +622,15 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      if (isset($form['#attached']) && isset($form['#attached']['placeholders'])) {
   +        $form['#attached']['placeholders'] = array_merge($form['#attached']['placeholders'], [$placeholder => $placeholder_render_array]);
   +      }
   +      $form['#attached']['placeholders'] = [$placeholder => $placeholder_render_array];
   

   All of this can be simplified to:

   $form['#attached']['placeholders'][$placeholder] = $placeholder_render_array;
   

Thanks for #20 @Wim Leers.

1. You're absolutely right, that was pretty useless, changed to the suggested $form_id
2. Confusingly when I removed the second array, I got the following error:
   #lazy_builder property must have an array as a value, containing two values: the callback, and the arguments for the callback.
   I solved this by adding an array with NULL as value. Is there an already established pattern on what argument to pass along when no actual arguments are needed?
3. Fixed.

This patch doesn't functionally change anything to #19, but that patch made a big difference in test failures as expected. The remaining test failures seem to largely be related to "The specified #ajax callback is empty or not callable.".

#22.2: just the empty array should do.

This fixes #23.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -613,7 +622,11 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
-      $form['#action'] = $this->buildFormAction();
+      $placeholder = hash('sha1', $form_id);
+      $placeholder_render_array = [
+        '#lazy_builder' => ['form_builder:renderPlaceholderFormAction', []],
+      ];
+      $form['#attached']['placeholders'][$placeholder] = $placeholder_render_array;

AFAICT this can't work, because the placeholder is never set anywhere :)

This needs:

$form['#action'] = $placeholder;

Running the tests with the change suggested in #26 locally makes take a very long time.
I cancelled a test that ususally takes at most 3 minutes (CommentPagerTest) after +- 25 minutes. Let's see if that's an issue on my machine only or that the bots agree this change is bad.

I also added more comments in the code so it better describes what's going on.

@borrison_ There's a bug when running tests in the UI that have a fatal error. The UI just runs them again and again, without visual feedback. Refresh the page if you think that's the problem and it will show an error the next time it completes.

Since the previous patch makes the testrunner very unhappy, I reverted the change there. Because I can't get any feedback from the testrunner (both locally and the testbot) that is clear enough for me to figure out I'm just going to leave this patch here as is. I'll try to come back to it in the following days.

I think I found out what was going wrong.
The renderable array I was returning from renderPlaceholderAction wasn't really a renderable array. This should be a better way to do this.
Locally, this fixes the BlockFormInBlockTest and the previously failing CommentPagerTest so that's a good sign.

Did a complete review. Looking great :) Almost there.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -603,6 +603,21 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
   +   * #lazy_builder callback; this renders a placeholder that's used in a form
   +   * action to use the current request's url
   

   * #lazy_builder callback; renders a form action URL.
   

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -603,6 +603,21 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
   +      '#cache' => ['max-age' => 0],
   

   Technically, this is actually perfectly cacheable: if you look at \Drupal\Core\Form\FormBuilder::buildFormAction(), you see it relies on the entire URL. So, let's change this line to:

   '#cache' => ['contexts' => ['url']],
   

3. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -612,7 +627,15 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      $placeholder = hash('sha1', $form_id);
   +      $placeholder_render_array = [
   +        '#lazy_builder' => ['form_builder:renderPlaceholderFormAction', []],
   +      ];
   

   Let's add a comment here:

   // Generate a placeholder and a render array to replace it.
   

   (This comment is then analogous with the one in RouteProcessorCsrf.)

   Also, in fact, we should NOT use the $form_id in the hash, let's just use the hardcoded string 'form_action' and concatenate Settings::getHashSalt() to prevent guessability.

   The beauty is then that all forms use the same placeholder. Which has as a nice consequence that if there are multiple forms on a page, that all of their action URLs will be set at the same time!

   (We should add a test for that as well.)

4. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -612,7 +627,15 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      // Attach the lazy builder to the form and add the placeholder in the form
   +      // action so the #lazy_builder callback knows what to replace.
   

   I think a clearer comment would be:

   // Instead of setting an actual action URL, we set
   // the placeholder, which will be replaced at the very last moment. This
   // ensures forms with dynamically generated action
   // URLs don't break cacheability.
   

   (This comment is then analogous with the one in RouteProcessorCsrf.)

5. +++ b/core/modules/block/src/Tests/BlockFormInBlockTest.php
   @@ -0,0 +1,94 @@
   +  protected function setUp() {
   

   @inheritdoc

6. +++ b/core/modules/block/src/Tests/BlockFormInBlockTest.php
   @@ -0,0 +1,94 @@
   +   * Test to see if form in block's redirect isn't cached
   

   Missing trailing period.

7. +++ b/core/modules/block/src/Tests/BlockFormInBlockTest.php
   @@ -0,0 +1,94 @@
   +    $this->drupalPostForm(NULL, $form_values, t('Submit'));
   

   This seems to be "floating". I think it belongs with the next code block? If so, let's put it just below the comment on the next code block?

8. +++ b/core/modules/block/src/Tests/BlockFormInBlockTest.php
   @@ -0,0 +1,94 @@
   +    // Make sure that submitting the form didn't redirect us to the first
   +    // page we submitted the form from.
   

   80 cols.

9. +++ b/core/modules/block/src/Tests/BlockFormInBlockTest.php
   @@ -0,0 +1,94 @@
   +    $this->assertText(t('Your email address is @email', ['@email' => 'test@example.com']));
   +
   +  }
   

   Extraneous \n.

10. +++ b/core/modules/block/tests/modules/block_test/src/Form/TestForm.php
    @@ -0,0 +1,56 @@
    +  public function buildForm(array $form, FormStateInterface $form_state) {
    +
    +    $form['email'] = [
    

    Extraneous \n.

11. +++ b/core/modules/block/tests/modules/block_test/src/Plugin/Block/TestFormBlock.php
    @@ -0,0 +1,31 @@
    +    $form = \Drupal::formBuilder()->getForm('Drupal\block_test\Form\TestForm');
    +
    +    return $form;
    

    Let's just return \Drupal::formBuilder() …

This patch fixes all the issues mentioned in #33.

Regarding .3, Not sure if there's a way to test that all forms use the same placeholder, or that they get replaced at the same time. I have included a new test that checks what happens when 2 forms are included on the same page. That should be enough to verify this?

1. +++ b/core/modules/block/src/Tests/BlockFormInBlockTest.php
   @@ -82,13 +78,50 @@ function testCachePerPage() {
   +   * When there are two forms on the same page, that
   

   Incomplete sentence.

2. +++ b/core/modules/block/src/Tests/BlockFormInBlockTest.php
   @@ -82,13 +78,50 @@ function testCachePerPage() {
   +  public function testMultipleFormsPerPage() {
   

   I think this test can be vastly simplified, because the other test already verifies that the original bug is fixed. This test does not need to repeat that test, it only needs to care about the placeholder behavior in case of multiple forms. So, something like:

   1. have a custom controller that returns two forms:
      

      $build = [
       'form_1' => $this->formBuilder->…,
       'form_2' => …,
      ];
      

   2. Also associate a #post_render callback with it like \Drupal\Tests\Core\Render\RendererRecursionTest::testRenderRecursionWithNestedRenderPlain() has, this runs after the HTML has been rendered, but before placeholders are replaced
   3. to verify that the placeholders will be replaced by a single #lazy_builder callback, the #post_render callback only needs to do drupal_set_message(array_keys($elements['#attached']['placeholders']), which should output a single string, which is the placeholder used by both
   4. have that #post_render callback check that there are *two* placeholders in the HTML, and that they are identical — i.e. identical to the string that the previous step emitted

(It's a bit ugly, but otherwise you'd have to carefully render the render array in stages. This is less work.)

Fixes remarks from #36 and test failures of the previous patch (#34).

I just noticed I didn't fix all issues of #33 earlier. I'm not yet using Settings::getHashSalt() to generate a placeholder. When I do, I get an error: (RuntimeException: Missing $settings['hash_salt'] in settings.php. in Drupal\Core\Site\Settings::getHashSalt()). I don't understand why this happens, the hash_salt is set in the settings file.

As mentioned in #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder, using crc32 instead of hash('sha1', ...) should be sufficient.

When changing the code, be sure to also change the test that accompanies that piece of code. Test should now no longer fail.

Patch is looking great :)

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -612,7 +627,17 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      // dynamically generated action URLs don't break cacheability.
   

   s/break/have poor/

2. +++ b/core/modules/block_content/src/Tests/BlockContentTranslationUITest.php
   @@ -18,6 +18,11 @@
   +  protected $defaultCacheContexts = ['languages:language_interface', 'theme', 'user.permissions', 'url'];
   

   Isn't this appearing in every ContentTranslationUITestBase subclass? If so, we should update the value in that base class instead.

#41.1: fixed.
#41.2: doesn't appear in TermTranslationUITest, CommentTranslationUITest, UserTranslationUITest. I think this override is a little bit more sane, but I can change it if you want.

#41.2: makes sense, thanks!

So, with that, I think this is pretty much ready; would be great if e.g. Fabianx could review this.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -603,6 +604,20 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
   +   *   A renderable array representing the form action.
   

   s/form action/form action URL/

2. +++ b/core/modules/block/src/Tests/BlockFormInBlockTest.php
   @@ -0,0 +1,98 @@
   +   * Test the actual placeholders
   

   Tests that all forms use the same placeholder.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -612,7 +627,17 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      $placeholder = crc32('form_action');
   

   Lets use:

   'form_action_' . crc32('form_action');

   in case someone tries to debug placeholders.

2. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -612,7 +627,17 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      $form['#attached']['placeholders'][$placeholder] = $placeholder_render_array;
   +      $form['#action'] = $placeholder;
   

   Nice! :) I love it!

3. +++ b/core/modules/block/src/Tests/BlockFormInBlockTest.php
   @@ -0,0 +1,98 @@
   +    $this->adminUser = $this->drupalCreateUser(['administer blocks', 'access administration pages']);
   +    $this->drupalLogin($this->adminUser);
   ...
   +    // Create additional users to test caching modes.
   ...
   +    $this->drupalLogin($this->normalUser);
   

   You can have two users logged in at the same time?

   Fascinating ...

   I actually think the 2nd login removes the first one.

   So probably the admin account is not needed at all for the test to pass?

Added the namespace for the placeholder.

Logging a user in was actually not needed at all the pass the test. Simplified the test.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -19,6 +19,7 @@
+use Drupal\Core\Site\Settings;

Unnecessary.

Otherwise, the rest of the patch looks good to me. A bit ugly, but that's the way the API works.

Attached patch fixes #46

Assuming this is an unrelated failure in ConfigImportInstallProfileTest. Retesting.

Ok, then I think this is ready now, per #44 and #46 :)

Otherwise, the rest of the patch looks good to me. A bit ugly, but that's the way the API works.

+1. The placeholder/#lazy_builder API was primarily designed for render arrays (i.e placeholders that'd be replaced with HTML)

We already have \Drupal\Core\Access\RouteProcessorCsrf::renderPlaceholderCsrfToken(). This will be another one. #2463567: Push CSRF tokens for forms to placeholders + #lazy_builder will be another one. All three need to render something other than HTML. Opened #2544220: Improve DX for non-HTML placeholders for that.

1. +++ b/core/lib/Drupal/Core/Form/FormBuilder.php
   @@ -612,7 +626,17 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
   +      $placeholder = 'form_action_' . crc32('form_action');
   

   We should use hash('crc32b') - that gives you a string hash.

   crc32() gives an signed int. And not only that but it's different on 32 vs. 64 bit, which would be fun given the placeholder will end up in the database and could potentially end up moving between those two.

   See the docs on http://php.net/manual/en/function.crc32.php for more.

2. +++ b/core/modules/block/tests/modules/block_test/src/Form/FavoriteAnimalTestForm.php
   @@ -0,0 +1,46 @@
   +   */
   +  public function buildForm(array $form, FormStateInterface $form_state) {
   +    $form['favorite_animal'] = [
   +      '#type' => 'textfield',
   +      '#title' => $this->t('Your favorite animal.')
   +    ];
   +
   

   Slightly missed opportunity to increase the zoological content of core via #options, although that would also make the form a bit restrictive.

Fixes #54.1, left the form as is for #54.2

So it looks like the previous patch was wrong, I had done the patch on a different branch and everything broke because of code from a different issue. Attaching what should be a correct patch and set this back to RTBC.

Sorry.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -612,7 +626,17 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+      // Generate a placeholder and a render array to replace it.
+      $placeholder = 'form_action_' . hash('crc32b', 'form_action');
+      $placeholder_render_array = [
+        '#lazy_builder' => ['form_builder:renderPlaceholderFormAction', []],
+      ];
+
+      // Instead of setting an actual action URL, we set the placeholder, which
+      // will be replaced at the very last moment. This ensures forms with
+      // dynamically generated action URLs don't have poor cacheability.
+      $form['#attached']['placeholders'][$placeholder] = $placeholder_render_array;
+      $form['#action'] = $placeholder;

Silly nitpick, but inlining the content of $placeholder_render_array would make the code a bit more fluent ?

Even more nitpicky : The explanation about *why* we use a placeholder would make more sense in the place where the "Generate a placeholder" code starts ? "Generate a placeholder and a render array to replace it" pretty much paraphrases the code below it and adds little value, it's the "why" that is interesting.

Feel free to ignore, esp. if #2544220: Improve DX for non-HTML placeholders comes and cleans up that code anyway.

I agree with @yched's silly nitpicks so I applied them. Not sure if that means I could've left this issue as RTBC so I moved it back to needs review.

+1, this reads much better!

@yched++

+++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
@@ -0,0 +1,43 @@
+    // Output all attached placeholders trough drupal_set_message, so we can see

nits: trough. And no parens on drupal_set_message()

+++ b/core/modules/block/tests/modules/block_test/src/Controller/TestMultipleFormController.php
@@ -21,7 +21,7 @@ public function testMultipleForms() {
-    // Output all attached placeholders trough drupal_set_message, so we can see
+    // Output all attached placeholders trough drupal_set_message(), so we can see

80 cols violation… but I think catch can fix that on commit.

So, #66 was passing first. For some reason it doesn't pass anymore though. I've been trying to figure out why.
I had some help from git bisect to pinpoint the commit where the failures are introduced and I think I've found it. So the attached patch is the same code as #66 with a git revert of the patch from #2501931: Remove SafeMarkup::set in twig_render_template() and ThemeManager and FieldPluginBase:advancedRender.

This patch is just to verify that all tests pass without that code, if it does, we know where to look.

@borisson_++

+++ b/core/lib/Drupal/Core/Theme/ThemeManager.php
@@ -318,10 +317,7 @@ public function render($hook, array $variables) {
-        // Theme functions do not render via the theme engine, so the output is
-        // not autoescaped. However, we can only presume that the theme function
-        // has been written correctly and that the markup is safe.
-        $output = SafeString::create($info['function']($variables));
+        $output = SafeMarkup::set($info['function']($variables));

@@ -391,7 +387,7 @@ public function render($hook, array $variables) {
 
-    return ($output instanceof SafeStringInterface) ? $output : (string) $output;

That reads like a regression, doesn't it? I think you should rebase your most recent patch.

Rebased the patch, this is basically the same patch as the one that passed in #60.

Attached patch will break RendererTest but that should be the only remaining failures.
This is nog a definitive solution but this should be a good indication to start figuring out where this broke.

The patch in #77 didn't apply anymore. The attached one does, still the same failures as in #77 though.

I still have the same reservations about the "solution" applied in #77 to resolve the errors.

First, straight reroll.

Here's the correct solution for #77.

And here are fixes for the last few failures. This should be green (unless #81 has failures in different tests than #79).

Next step:

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -638,6 +638,20 @@ public function processForm($form_id, &$form, FormStateInterface &$form_state) {
+      '#cache' => ['contexts' => ['url']],

['url'] is inaccurate; what is actually being varied on is ['url.path', 'url.query_args']. That's less specific than 'url', and hence more optimal.

Addressed my own remarks from #83.

I noticed that the rebase was incorrect in BlockContentTranslationUITest so I expect all patches above to come back red.

This looks great to me! And is hopefully way better than having to cache all blocks that contain a form by url.path and url.query_args ...

So there were new failures. This fixes those.

IS updated.

At this point in time, blocker for #2429617: Make D8 2x as fast: Dynamic Page Cache: context-dependent page caching (for *all* users!) = Critical, so promoting.

Back to RTBC. the test changes make sense.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -647,7 +661,15 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+      $placeholder = 'form_action_' . hash('crc32b', 'form_action');

This line is a bit confusing, why would we care about the hash at all? Why would 'form_action' be in any level less specific. Don't we want to include some information of the form onto the placeholder so we have a unique placeholder across forms?

#90: #33.3 answers this:

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -612,7 +627,15 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+      $placeholder = hash('sha1', $form_id);
+      $placeholder_render_array = [
+        '#lazy_builder' => ['form_builder:renderPlaceholderFormAction', []],
+      ];

Let's add a comment here:

// Generate a placeholder and a render array to replace it.

(This comment is then analogous with the one in RouteProcessorCsrf.)

Also, in fact, we should NOT use the $form_id in the hash, let's just use the hardcoded string 'form_action' and concatenate Settings::getHashSalt() to prevent guessability.

The beauty is then that all forms use the same placeholder. Which has as a nice consequence that if there are multiple forms on a page, that all of their action URLs will be set at the same time!

(We should add a test for that as well.)

We have a test for that. But for some reason we lost the salt along the way.

So needs work?

#90: Good question, we should probably add a comment there.

The reason is that we don't want 'form_action_' on this page to be replaced with '/node/2504139'.

The crc32b would still allow me to write a comment, where I use 'form_action_f0e3' and it gets replaced with '/node/2504139', so there is no security protection here, but its unlikely that happens during normal text.

We could theoretically make the placeholder be: 'action="form_action_$hash"' that we then replace with more, but not sure that really helps.

Usually the hash should be sufficiently different.

#94: +1 for NW.

All fixed. The tricky bits you saw I copied from the patch in #2478483: Introduce placeholders (#lazy_builder) to replace #post_render_cache.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -664,7 +665,14 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+        // The installer users forms, no hash salt is defined yet at that time.

Nitpick: uses instead of users, you are drupaldoomed!

I need to go AFK.

Better do so!

Related to #102, we seem to have a mix of hashing algorithms and seeds for placeholders in HEAD plus this patch:

FormBuilder: 
$placeholder = 'form_action_' . hash('crc32b', 'form_action' . $hash_salt);

RouteProcessorCsrf: 
$placeholder = hash('sha1', $path);

Renderer:
$attributes = new Attribute();
$attributes['callback'] = $placeholder_render_array['#lazy_builder'][0];
$attributes['arguments'] = UrlHelper::buildQuery($placeholder_render_array['#lazy_builder'][1]);
$attributes['token'] = hash('crc32b', serialize($placeholder_render_array));
$placeholder_markup = SafeMarkup::format('<drupal-render-placeholder@attributes></drupal-render-placeholder>', ['@attributes' => $attributes]);
   
FilterProcessResult:
$attributes = new Attribute();
$attributes['callback'] = $callback;
$attributes['arguments'] = UrlHelper::buildQuery($args);
$attributes['token'] = hash('sha1', serialize([$callback, $args]));
$placeholder_markup = Html::normalize('<drupal-filter-placeholder' . $attributes . '></drupal-filter-placeholder>');

I think a dedicated service for generating placeholders would be good. Perhaps with 2 methods, such as asHtml() (for the cases where the placeholder can be HTML markup, like the bottom two examples above) and asIdentifier() (for the cases where the placeholder needs to appear inside an attribute value and not be altered by functions such as Html::escape(), Html::cleanCssIdentifier(), UrlHelper::stripDangerousProtocols(), etc., like the top two examples). Then such a service could be well documented for why the chosen hashing algorithm and seed have the appropriate level of security.

But, if all that is too much scope for this issue, I think just matching what's already in HEAD for RouteProcessorCsrf and using something like hash('sha1', $form_id); could be good enough for now.

I agree that we should not duplicate the code all over core for that and I think the placeholdering service should indeed provide that.

However I also agree with #103 that that is a non-critical follow-up, because one of the auto-placeholdering patches adds a PlaceholderGenerator service like that anyway.

I however also agree with #102 that any random suffix would be enough for now.

Working on a patch addressing #102. I'll create a follow-up issue for #103.

Created #2562341: Add non-HTML placeholder generation to PlaceholderGenerator or a new service to address #103.

+++ b/core/lib/Drupal/Core/Form/FormBuilder.php
@@ -665,14 +664,9 @@ public function prepareForm($form_id, &$form, FormStateInterface &$form_state) {
+      // Use the proper API to generate the placeholder, when we have one. See
+      // https://www.drupal.org/node/2562341.
+      $placeholder = 'form_action_' . hash('sha1', $form_id);

I think the point was that we should use the same placeholder for all the forms, see #2504139-91: Blocks containing a form include the form action in the cache, so they always submit to the first URL the form was viewed at

Maybe this, then?

#103: We've been migrating all placeholders from using SHA1 to using CRC32. We must've missed one instance.

#104:
+1 for a non-critical follow-up.
+1 for a PlaceholderGenerator service, which incidentally is exactly what #2543334: Auto-placeholdering for #lazy_builder with bubbling of contexts and tags is adding (as Fabianx was already indicating).

#106: Thanks for filing that!

#108: That looks fine to me :) Let's see what testbot thinks.

I overlooked it first, but #108 uses SHA1, it should use CRC32 (hash('crc32b', …)).

Hope it's ok now :)

RTBC!

Looks great now! :)

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -179,7 +179,7 @@ protected function renderPlaceholder($placeholder, array $elements) {
-    $elements['#markup'] = str_replace($placeholder, $markup, $elements['#markup']);
+    $elements['#markup'] = SafeString::create(str_replace($placeholder, $markup, $elements['#markup']));

This doesn't need to be here... It can go in the Renderer::render - here...

if ($is_root_call) {
      $this->replacePlaceholders($elements);
      // @todo remove as part of https://www.drupal.org/node/2511330.
      if ($context->count() !== 1) {
        throw new \LogicException('A stray drupal_render() invocation with $is_root_call = TRUE is causing bubbling of attached assets to break.');
      }
      // Add this here... with a comment as to why.
      $elements['#markup'] = SafeString::create($elements['#markup']);
    }

From @Wim Leers in IRC

renderPlaceholder() method will become public at some point, to allow e.g. BigPipe to render a specific placeholder. Then it must also always result in a safe string.

That is a good reason to do that there.

Committed feb0fc2 and pushed to 8.0.x. Thanks!

+++ b/core/lib/Drupal/Core/Render/Renderer.php
@@ -179,7 +179,7 @@ protected function renderPlaceholder($placeholder, array $elements) {
     // Replace the placeholder with its rendered markup, and merge its
     // bubbleable metadata with the main elements'.
-    $elements['#markup'] = str_replace($placeholder, $markup, $elements['#markup']);
+    $elements['#markup'] = SafeString::create(str_replace($placeholder, $markup, $elements['#markup']));
     $elements = $this->mergeBubbleableMetadata($elements, $placeholder_elements);

So I think we're missing test coverage of this. However, it's only indirectly part of the critical so let's do that in another issue.

I seem to have a problem with this.

This caused test fails in simplenews: https://qa.drupal.org/pifr/test/937688

For some reason, it seems that the lazy builder is not called and the form action is *not* updated. The result is that the form is posted to the placeholder string (POST http://d8/form_action_cc611e1d returned 404 (5.45 KB).). I don't know yet why this happen and what's different, how could this not be called?

Any help appreciated.

I have the same problem as #118 but with all regular node forms, $form['action'] is NULL so I guess normally it would submit the form the current page, but right now the form action is set to form_action_cc611e1d so it submits to node/add/form_action_cc611e1d and it 404's. (Running BETA15)

#119: I tested beta15 on simplytest.me and could not reproduce that problem. Any ideas or steps to reproduce?

I don't know about #119, it obviously doesn't seem to happen for everyone, but the simplenews test failures are consistent on at least three different environments (testbot, local, d8status), so that should be reproducable. And I'd guess if we can figure that out then that will also help with the problem described in #118.

I did find a problem with lazy builders in #2565501: Tests are not working because Dynamic page cache was committed in core., see my comment with the patch, but I don't think it's the same here.

It's on a content type with quite a few fields from contrib modules that I have patched though (geofield, paragraphs). I'll do a few tests removing some of those fields and will report back.

Edit: it seems to be a problem with the geofield module.

Quick update if someone is running into a similar problem.

In the simplenews case, it was because some custom code added another #attached element and didn't correctly merge in existing #attached elements. Worked fine before since there weren't any but now there are.

I have the next case in a module: In hook_form_alter() I need the real value of $form['#action'] but I'm getting the form_action_* placeholder instead. How can I get the real URL?

Finally I did this but I don't like it:

function example_form_alter(...) {
  ...
  if (Unicode::strpos($form['#action'], 'form_action_') === 0) {
    $render_array = \Drupal::formBuilder()->renderPlaceholderFormAction();
    $action = $render_array['#markup'];
  }
  else {
    $action = $form['#action'];
  }
  ...
}

I think buildFormAction() should go in the FormBuilderInterface as public method.

This caused a regression elsewhere in core due to missing test coverage: #2649404: Form API's #https is broken for user login block (no longer opts in form to use HTTPS action URL). #2342593: Remove mixed SSL support from core (or earlier) should've added test coverage, that'd have prevented this regression from being introduced.