Allow configuring of manual checking per field

Thanks for this, seems to work well from my initial testing and is badly needed for content types that are heavy on the paragraph side.

A potential nice to have would be to have a global option rather than per field.

Noticing the following on node view after create/add

Notice: Undefined index: edit_manual_checking in Drupal\yoast_seo\Plugin\Field\FieldWidget\YoastSeoWidget->massageFormValues() (line 179 of modules/contrib/yoast_seo/src/Plugin/Field/FieldWidget/YoastSeoWidget.php).
Drupal\yoast_seo\Plugin\Field\FieldWidget\YoastSeoWidget->massageFormValues(Array, Array, Object) (Line: 381)
Drupal\Core\Field\WidgetBase->extractFormValues(Object, Array, Object) (Line: 231)
Drupal\Core\Entity\Entity\EntityFormDisplay->extractFormValues(Object, Array, Object) (Line: 338)
Drupal\Core\Entity\ContentEntityForm->copyFormValuesToEntity(Object, Array, Object) (Line: 304)
Drupal\Core\Entity\EntityForm->buildEntity(Array, Object) (Line: 159)
Drupal\Core\Entity\ContentEntityForm->buildEntity(Array, Object) (Line: 190)
Drupal\Core\Entity\ContentEntityForm->validateForm(Array, Object)
call_user_func_array(Array, Array) (Line: 82)
Drupal\Core\Form\FormValidator->executeValidateHandlers(Array, Object) (Line: 275)
Drupal\Core\Form\FormValidator->doValidateForm(Array, Object, 'node_flexible_page_form') (Line: 118)
Drupal\Core\Form\FormValidator->validateForm('node_flexible_page_form', Array, Object) (Line: 576)
Drupal\Core\Form\FormBuilder->processForm('node_flexible_page_form', Array, Object) (Line: 319)
Drupal\Core\Form\FormBuilder->buildForm('node_flexible_page_form', Object) (Line: 61)
Drupal\Core\Entity\EntityFormBuilder->getForm(Object) (Line: 129)
Drupal\node\Controller\NodeController->add(Object)
call_user_func_array(Array, Array) (Line: 123)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 582)
Drupal\Core\Render\Renderer->executeInRenderContext(Object, Object) (Line: 124)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->wrapControllerExecutionInRenderContext(Array, Array) (Line: 97)
Drupal\Core\EventSubscriber\EarlyRenderingControllerWrapperSubscriber->Drupal\Core\EventSubscriber\{closure}() (Line: 151)
Symfony\Component\HttpKernel\HttpKernel->handleRaw(Object, 1) (Line: 68)
Symfony\Component\HttpKernel\HttpKernel->handle(Object, 1, 1) (Line: 57)
Drupal\Core\StackMiddleware\Session->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\KernelPreHandle->handle(Object, 1, 1) (Line: 106)
Drupal\page_cache\StackMiddleware\PageCache->pass(Object, 1, 1) (Line: 85)
Drupal\page_cache\StackMiddleware\PageCache->handle(Object, 1, 1) (Line: 47)
Drupal\Core\StackMiddleware\ReverseProxyMiddleware->handle(Object, 1, 1) (Line: 52)
Drupal\Core\StackMiddleware\NegotiationMiddleware->handle(Object, 1, 1) (Line: 23)
Stack\StackedHttpKernel->handle(Object, 1, 1) (Line: 693)
Drupal\Core\DrupalKernel->handle(Object) (Line: 19)

I think we're missing Schema changes in the patch

This perhaps needs more thought, the above notice is firing because 'edit_manual_checking' forms part of the element itself, as opposed to a value hence being an undefined index. I am attaching a patch that seems to work in my implementation.

On initial load of the entity no analysis is performed and the SEO status is therefore not correctly shown. You can correct this by pushing the button, but it is irritating that the status quo is not correctly displayed. For example you see the status "Good" in the content overview and then while editing the content the status is shown as "Bad" (until the button is pressed). So I would propose to have an initial analysis automatically.

This patch should help with this. It is also taking care that the inital analysis is not performed for new content, because it makes no sense there.

Looks good, thanks!

+++ b/src/Plugin/Field/FieldWidget/YoastSeoWidget.php
@@ -226,6 +235,10 @@ class YoastSeoWidget extends WidgetBase implements ContainerFactoryPluginInterfa
+      $summary[] = 'Manual checking enabled';

Use $this->t('Manual checking enabled')

This features seems really great especially for paragraphs user and ajax problems we have, could you add it to the next release ?

Attached is an updated patch with the fix suggested by mpp.
I also tried creating an merge request with that new drupal.org feature: https://git.drupalcode.org/project/yoast_seo/-/merge_requests/1
Not sure what the maintainers of this module prefer, so adding both :)

Whohoo, we have merge requests! Aaand inline code suggestions, time to party!

+ if(!this.config.is_new) {
+ this.$form.find('.yoast-seo-preview-submit-button').mousedown();
+ }

=> is missing a space. Thanks @sven fot the update, I added an inline code suggestion for this.

Other than that this looks good, lgtm!

I tried applying the inline code suggestion in the MR, but that throws AJAX errors. Guess this feature is still too cutting edge right now :(

OK fixed the space now.. Not sure if you will get credit for that space or not @mpp :)

Hehe, me neither. Thanks @sven, exciting :-)

Might still need some work:

Hi Sven and MPP,

Thanks for the proposed changes. There's a few things I would like to see if we can address, besides that I still think the automatic updating is a very useful feature, so if at all possible I would like to aim to preserve it.

Exploring an option with automatic updates

The core issue here stems from the usage of Drupal's AJAX API which does a psuedo form-submit and upon response updates the form. The form submit method is used because the form API does a lot of heavy lifting for us in terms of making entities available. Previously we would do that manually but this would cause a lot of issues.

This means that a form submit is needed. However, from what I understand of the issue, the problem is not necessarily that a form submit happens at all. The issue here is that the AJAX JavaScript subsystem will update the form when it gets a response.

There are two possible solutions that I see here. Either the logic on the server side is changed to change what the form processing sends back to the client. The alternative is to change the client side: either how the AJAX request is sent, or what happens to the response. I think changing the JavaScript code on the client is easiest.

The set-up of AJAX based on the #ajax callback in the render property happens in the Render Element base class. The settings configured there are picked up in the ajax.es6.js attach function.

We can either change how we attach AJAX, doing it manually to allow our own set-up to be done client side. This allows us to mimick the submission while not using Drupal's default response handler which alters the page.

Alternatively we can monkeypatch the functions on Drupal.AjaxCommands.prototype which receive the ajax request. The monkeypatched version can use the ajax object to figure out if this is a request for the Real-Time SEO module and filter out those commands, passing on anything else. The only command that will need to be preserved is the event trigger on the body element that contains the new analysis data.

Either of these solutions should remove the side-effects that are currently associated with analysing an entity. This should help avoid clashes with any interactive events, not just paragraphs.

Feedback on the fix for manual updates

Although I'd like to explore the above options before merging anything, I have some feedback on the current proposed implementation.

1. The change in js/yoast_seo.js removes the use of refreshData. While I do think the added is_new check is a good idea, I don't think we should bypass the check for other AJAX events that refreshData does, so just wrapping that in the is_new check is probably better.

2. I don't think the this.config.manual_checking should happen in the refreshData function. Instead we should use that configuration to decide whether or not to apply the event listeners at all.

3. We should probably just use JavaScript to show the submit button on the form, by reading the config from the DrupalSettings. This avoids adding an arbitrary unsupported #property to the form array. It also ensures that the button is only shown after everything has loaded and won't show when JavaScript is disabled (which is desired since the checking doesn't work without JavaScript anyway).

Is it possible to write a test for the bug that's described in this issue? That would help us test out various scenarios and avoid regressions in the future.

Forgot to match the status to my comments.

I've dug into this a bit deeper. It isn't actually caused by the Real-Time SEO module but it's a limitation of the paragraphs module.

The Real-Time SEO module provides two commands as a response to its update request. Those commands are invoke -- which doesn't touch the form but just calls a callback -- and update_build_id -- which changes a value of the form_build_id input, but doesn't otherwise alter fields in the form.

To prevent doubt, I was able to filter out the update_build_id using a Proxy but even then the problem persisted.

Finally I've removed the Real-Time SEO field altogether and am still able to reproduce the issue as follows:

1. In InlineParagraphsWidget::addMoreSubmit add a sleep(10); to reliably simulate that adding paragraphs is slow
2. Create a node type with a paragraphs field
3. Open the node create form to create a new node with this field
4. Click on the "Add paragraph" button in the node form
5. Type something in an existing paragraphs field.
6. Observe that content added after clicking "Add paragraph" is wiped out when the new paragraph is added

This is caused by the insert command from the Paragraphs module sending the entire paragraphs field to the server and returning it as code to be replaced. This means your original input is captured in the paragraphs request and then enforced after. This is not caused by the Real-Time SEO module, since it's field isn't present.

With that in mind I'm marking this as "Postponed (maintainer needs more info)" but unless someone can point to what exactly in the Real-Time SEO module is causing this (rather than an issue in Paragraphs), I'm leaning towards closing this as "won't fix".

The following issues for Paragraphs also provide some more background information on the general performance issue in Paragraphs and further demonstrate this is not Real-Time SEO specific:

• #2935134: [meta] Improve performance of paragraphs widget
• #2829716: Multiple AJAX actions at once breaks paragraphs fields

The following Drupal core issue would stop the above scenario from being allowed in Paragraphs #2830295: Concurrent ajax submits cause user data loss.

------------

if anyone is interested, below is the code to show what commands are being executed. This was added for debugging in Drupal.behaviors.yoast_seo.attach

      // Monkey patch the commands. This allows the commands to work as normal
      // but lets us spy on the commands that are being executed.
      for (const instance of Drupal.ajax.instances) {
        if (!instance || instance.commands.__proxy) {
          continue;
        }
        instance.commands = new Proxy(
          instance.commands,
          {
            get: (oTarget, sKey) => {
              // This property is used as a marker to prevent attaching the 
              // proxy multiple times.
              if (sKey === "__proxy") {
                return true;
              }

              if (typeof oTarget[sKey] === "function") {
                // As an experiment, prevent changing the build_id. This doesn't
                // actually
                if (instance.selector.indexOf("yoast-seo-preview") !== -1 && sKey === "update_build_id") {
                  return () => {};
                }

                // Log the arguments, selector and command name and then execute
                // the command, propagating the result.
                return function () {
                  console.log(arguments, instance.selector, sKey);
                  return oTarget[sKey].apply(oTarget, arguments);
                }
              }

              return oTarget[sKey];
            }
          }
        );
      }

Thanks for the elaborate investigation Kingdutch!

However the site we experienced this issue with, does not use the Paragraphs module at all.
The content type where this error occurs is fairly simple, with just a few texfields.
The only thing that uses AJAX is the Media image field, which loads the media gallery.

If this is not a yoast_seo related issue, then it would be a Drupal core issue, given the above scenario?
Unless we have another module installed that intervenes here unknowingly.

Update:
When using the alpha5 release on my site, this issue seems to have disappeared.
Will investigate further if this actually solved things or not.

When loading a node edit page with the yoast seo field on it, then filling in a focus keyword and click outside that field (blur), it only shows the site URL with "undefined" appended to it. E.g. https://www.mywebsite.comundefined. Clearly something goes wrong in the JS.

I think when using the manual button, the blur functionality on the focus keyword field can be removed?

Clicking the SEO preview button again shows the example and analysis. Clicking the SEO preview button after page load without entering a keyword also generates the example + analysis.

Not sure if it's related but this started happening around the time we switched to the Gin admin theme. Probably not.

Rerolled patch for 8.x-2.0-alpha9

Created patch from current MR and updated it so the form change listener is not initiated when manual checking is enabled. This way the change on the "focus keyword" is ignored. See comment #26. Feel free to add this to the MR.

This is not working with 2.1.0

I'm closing this as outdated. There have been a few changes to the module since this issue was opened.

Using the view mode configuration you can now decide exactly which fields are being included in the analysis. Additionally as part of #3008802: The page freezes while the Real-Time SEO module runs script, there is now a button that allows you to manually decide when to trigger an analysis.

Those two things should cover the scope of this issue. If that's not the case, please open a new issue.