PHP warning when saving text format with CKE5: $form_state->getTriggeringElement() unexpectedly returning NULL

What about something like this?

Maybe this?

Otherwise, maybe something like

$this->assertSame('', file_get_contents($this->root . '/' . $this->siteDirectory . '/error.log'));

I don't have specific steps yet, but the fix is clear:
$form_state->getTriggeringElement() can return array or NULL, and that needs to be taken into account here.

Of note: For whatever reason, I was able to reproduce this on PHP 8.0 but not on PHP 7.3. The STR are:

1. Install Standard and the CKEditor 5 module.
2. Create a text format, granting administrators access, and select CKE5 as the editor for the format.
3. Save the text format.
4. Edit the text format you just created.
5. Click save again (no changes needed).
6. The error will appear onscreen if the site is configured to show warnings and notices; otherwise, check watchdog and it will be there.

Lauri was using PHP 8.0 and others were not, which explained the issues reproducing it.

The fact that the behavior is different on PHP 7.3 versus 8.0 is why we were concerned that we might have stumbled onto some regression with the FormState API.

Looks like I’m almost at the point where I can catch PHP notices in functional JS tests.

This also looks like something that belongs in Drupal core… so once I get it working I’ll file a core issue 👍

NICE! That is working 🥳🤓 — that does mean this merits a Drupal core issue… so tagging Needs followup.

Now … let's figure out whether this is indeed a PHP 8-only problem. Queuing tests for PHP 7.3 & 7.4. I bet it's a problem starting with PHP 7.4.

Funny, the 7.3 tests catch a different PHP notice:
if (!(bool) $editor->getImageUploadSettings()['status']) {
causes
Notice: Undefined index: status in Drupal\ckeditor5\Plugin\CKEditor5PluginManager->isPluginDisabled()

:D

Oh … hah :D That's indeed absolutely not what I expected :P

I can't believe we don't have a better way to fail tests on notices and warnings. Like that seems fundamentally broken at a basic level. We shouldn't have to check watchdog (although the method is pretty cool as a workaround).

Asked about this and got the following info from @larowlan:

larowlan:
\Drupal\Core\Test\HttpClientMiddleware\TestHttpClientMiddleware::__invoke

tl;dr the site under test should communicate php errors back to the runner via `X-Drupal-Assertion` headers

xjm:
So this converts warnings etc. into exceptions to raise fails, but how do the headers get set in the first place?

larowlan:
by the error handler

see _drupal_error_header

which is called from _drupal_log_error

so if you could ask him to debug inside _drupal_log_error - there may be something preventing that hunk from running in this scenario, which we need to fix

Also this warning was interfering with debugging on #3228477: CKEditor5PluginConfigurableInterface should extend PluginFormInterface and ConfigurableInterface instead of adding ::settingsForm(), so I committed a temporary workaround (pointing to this issue) that got merged in that MR:
https://git.drupalcode.org/project/ckeditor5/-/commit/50852d5802f4e9f8af...

I can't believe we don't have a better way to fail tests on notices and warnings. Like that seems fundamentally broken at a basic level. We shouldn't have to check watchdog (although the method is pretty cool as a workaround).

+1

Which is why I wrote in #8:

This also looks like something that belongs in Drupal core… so once I get it working I’ll file a core issue 👍

so if you could ask him to debug inside _drupal_log_error - there may be something preventing that hunk from running in this scenario, which we need to fix

I'm 90% certain that this simply never worked for functional JS tests. It's based on the user agent that the relevant middleware gets registered — \Drupal\Core\CoreServiceProvider::registerTest() does:

  /**
   * Registers services and event subscribers for a site under test.
   *
   * @param \Drupal\Core\DependencyInjection\ContainerBuilder $container
   *   The container builder.
   */
  protected function registerTest(ContainerBuilder $container) {
    // Do nothing if we are not in a test environment.
    if (!drupal_valid_test_ua()) {
      return;
    }
    // The test middleware is not required for kernel tests as there is no child
    // site. DRUPAL_TEST_IN_CHILD_SITE is not defined in this case.
    if (!defined('DRUPAL_TEST_IN_CHILD_SITE')) {
      return;
    }
    // Add the HTTP request middleware to Guzzle.
    $container
      ->register('test.http_client.middleware', 'Drupal\Core\Test\HttpClientMiddleware\TestHttpClientMiddleware')
      ->addTag('http_client_middleware');
  }

… but when using functional JS tests, drupal_valid_test_ua() never returns TRUE because

  protected function prepareRequest() {
    $session = $this->getSession();
    $session->setCookie('SIMPLETEST_USER_AGENT', drupal_generate_test_ua($this->databasePrefix));
  }

only runs when using $this->drupalGet(…), not when the browser itself makes AJAX requests due to e.g. typing something or clicking something.

That's my theory at least 🙈

Testing my theory: #3228956: PHP notices are not detected for headless browser-triggered requests in FunctionalJavascript tests.

@tim.plunkett already committed a work-around in https://git.drupalcode.org/project/ckeditor5/-/commit/50852d5802f4e9f8af....

That means that the only thing to do here now is to A) prevent regressions, B) figure out if this is actually a deeper bug in Drupal core. For B), I've opened #3228956: PHP notices are not detected for headless browser-triggered requests in FunctionalJavascript tests. Postponing on that now.

Will push this forward on Monday.

Fully confirmed over at #3228956: PHP notices are not detected for headless browser-triggered requests in FunctionalJavascript tests.

Per #3228956-9: PHP notices are not detected for headless browser-triggered requests in FunctionalJavascript tests and #3228956-12: PHP notices are not detected for headless browser-triggered requests in FunctionalJavascript tests, it's not clear yet how we want to address this in Drupal core. We should not wait for that. Instead, we can adopt the solution that Thunder uses per #3228956-10: PHP notices are not detected for headless browser-triggered requests in FunctionalJavascript tests, which is effectively a cleaned up version of the hack I applied here :) And we can simplify what they did, because … we do not want anything logged by PHP.

Nice, #24 failed on the PHP notice that @tim.plunkett mentioned in #12 — but now in PHP 8 :) Great!

@tim.plunkett already fixed the originally reported PHP notice in #3228477: CKEditor5PluginConfigurableInterface should extend PluginFormInterface and ConfigurableInterface instead of adding ::settingsForm() as he mentioned in #16.

If this comes back green, I'm going to go ahead and merge this.

+++ b/tests/src/FunctionalJavascript/CKEditor5TestBase.php
@@ -42,6 +47,40 @@ abstract class CKEditor5TestBase extends WebDriverTestBase {
+   * @todo Remove in https://www.drupal.org/project/drupal/issues/3228956.

This ensures we'll remove it when the core issue lands :)