Problem/Motivation

The tests
core/modules/ckeditor5/tests/src/Kernel/ValidatorsTest.php

both fail locally for me.

Testing Drupal\Tests\ckeditor5\Kernel\ValidatorsTest
..............W                                                   15 / 15 (100%)

Time: 19.87 seconds, Memory: 10.00 MB

There was 1 warning:

1) Warning
The data provider specified for Drupal\Tests\ckeditor5\Kernel\ValidatorsTest::testPair is invalid.
Symfony\Component\Yaml\Exception\ParseException: File "profiles/standard/config/install/filter.format.restricted_html.yml" does not exist.

Testing Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest
....F....                                                           9 / 9 (100%)

Time: 12.66 seconds, Memory: 8.00 MB

There was 1 failure:

1) Drupal\Tests\ckeditor5\Kernel\SmartDefaultSettingsTest::test with data set "restricted_html can be switched to CKEditor 5 after dropping the two markup-creating filters (3 upgrade messages)" ('restricted_html', array(true, true), array(array(array('heading', 'bold', 'italic', '|', 'link', 'blockQuote', 'code', 'bulletedList', 'numberedList', 'sourceEditing')), array(array(array('heading2', 'heading3', 'heading4', 'heading5', 'heading6')), array(array('<cite>', '<dl>', '<dt>', '<dd>', '<a hreflang>', '<blockquote cite>', '<ul type>', '<ol start type>', '<h2 id>', '<h3 id>', '<h4 id>', '<h5 id>', '<h6 id>')))), '<br> <p>', array(array('CKEditor 5 only works with HT...ymore.', 'CKEditor 5 only works with HT...ymore.')), array('The following plugins were en...</em>.', 'The following tags were permi...d&gt;.', 'This format's HTML filters in...d&gt;.'), array('CKEditor 5 needs at least the...ilter.'))
Failed asserting that two arrays are identical.
--- Expected
+++ Actual
@@ @@
 Array &0 (
     '' => Array &1 (
-        0 => 'CKEditor 5 only works with HTML-based text formats. The "<em class="placeholder">Convert URLs into links</em>" (<em class="placeholder">filter_url</em>) filter implies this text format is not HTML anymore.'
-        1 => 'CKEditor 5 only works with HTML-based text formats. The "<em class="placeholder">Convert line breaks into HTML (i.e. &lt;code&gt;&amp;lt;br&amp;gt;&lt;/code&gt; and &lt;code&gt;&amp;lt;p&amp;gt;&lt;/code&gt;)</em>" (<em class="placeholder">filter_autop</em>) filter implies this text format is not HTML anymore.'
+        0 => 'CKEditor 5 only works with HTML-based text formats. The "<em class="placeholder">Convert line breaks into HTML (i.e. &lt;code&gt;&amp;lt;br&amp;gt;&lt;/code&gt; and &lt;code&gt;&amp;lt;p&amp;gt;&lt;/code&gt;)</em>" (<em class="placeholder">filter_autop</em>) filter implies this text format is not HTML anymore.'
+        1 => 'CKEditor 5 only works with HTML-based text formats. The "<em class="placeholder">Convert URLs into links</em>" (<em class="placeholder">filter_url</em>) filter implies this text format is not HTML anymore.'
     )
 )

Steps to reproduce

Run the tests locally.

Proposed resolution

  • Use a path relative to the test.
  • Work out how to ensure a consistent order.

If I fix the path in ValidatorsTest then it fails for the same reason as SmartDefaultSettingsTest so going to fix both things here.

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#9 3249240-8.patch8.29 KBalexpott
#9 4-8-interdiff.txt3.4 KBalexpott
#4 2-3-interdiff.txt5.23 KBalexpott
#4 3249240-3.patch5.99 KBalexpott
#2 3249240-2.patch1.06 KBalexpott
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

alexpott created an issue. See original summary.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
FileSize
3249240-2.patch1.06 KB

Here's the path fix... let's see what happens on DrupalCI.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Active » Needs review
Parent issue: » #3238333: Roadmap to CKEditor 5 stable in Drupal 9
alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Title: CKEditor 5 tests fail locally. » CkEditor needs validate the filters in the correct order - CKEditor 5 tests fail locally.
Priority: Normal » Critical
FileSize
2-3-interdiff.txt5.23 KB
3249240-3.patch5.99 KB

Gonna make this a critical. I think filter plugins must be validated in the correct order and I'm not sure that this happening at the moment. Whether this is the correct fix I'm not sure but at least now the order is determined by the sorting of filter plugins on a filter format.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

Thank you for #2 it allows to start a test at least

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Priority: Critical » Normal
Status: Needs review » Reviewed & tested by the community
Related issues: +#3249240: HTMLRestrictionsUtilities:: providedElementsAttributes() causes deprecations on PHP 8.1

#4 works for me

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Priority: Normal » Critical
andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Issue tags: +PHP 8.1

as it became visible on PHP 8.1 version

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Needs review
Issue tags: -PHP 8.1
FileSize
4-8-interdiff.txt3.4 KB
3249240-8.patch8.29 KB

@andypost #4 definitely works but I think we need to decide whether it is the best fix. I think it is a bit odd that FilterFormat::filters() does not currently return sorted filters. If this is the correct fix then we need to has explicit test coverage of this.

Here's a more elegant fix that only makes changes to CkEditor5 module code. The plugin collection is correctly sorted - it's just that ->getAll() is not. Maybe we need to add followup for that but the docs for getAll() say:

  /**
   * Retrieves filter definitions and creates an instance for each filter.
   *
   * This is exclusively used for the text format administration page, on which
   * all available filter plugins are exposed, regardless of whether the current
   * text format has an active instance.
   *
   * @todo Refactor text format administration to actually construct/create and
   *   destruct/remove actual filter plugin instances, using a library approach
   *   à la blocks.
   */
  public function getAll() {

Given it's intent to be exclusively used for the UI let's not use it here. We don't need to. We can yield the values we want from the iterator.

@andypost this has nothing to do with PHP 8.1 - it happens locally for me regardless of PHP version.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented

This approach looks better, just not sure about doc-blocks for iteratable

++ to approach because this method is private and the code is new 

+++ b/core/modules/ckeditor5/src/Plugin/Validation/Constraint/FundamentalCompatibilityConstraintValidator.php
@@ -194,25 +192,21 @@ private function checkHtmlRestrictionsMatch(EditorInterface $text_editor, Fundam
+   * @return iterable|\Drupal\filter\Plugin\FilterInterface[]
+   *   An iterable of matched filter plugins.
...
+        yield $id => $filter;

it returns $id => $entity but doc-block mentions only entity interface

Status: Needs review » Needs work

The last submitted patch, 9: 3249240-8.patch, failed testing. View results

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Needs work » Needs review

@andypost the phpdoc is inline with other generators in the codebase - for example:

    /**
     * Returns an array of bundles to register.
     *
     * @return iterable|BundleInterface[] An iterable of bundle instances
     */
    public function registerBundles();

And inline with people's thoughts on https://stackoverflow.com/questions/22272280/proper-phpdoc-comment-for-i...

Maybe you missed the iterable| at the beginning of @return.

andypost’s picture

andypost
he/him
Primary language Russian
CreditAttribution: andypost as a volunteer and at Skilld commented
Status: Needs review » Reviewed & tested by the community

Overlooked at late night, thank you for elaboration, then I guess it good to go

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Title: CkEditor needs validate the filters in the correct order - CKEditor 5 tests fail locally. » CKEditor 5 needs validate the filters in the correct order - CKEditor 5 tests fail locally.
Assigned: Unassigned » Wim Leers
Status: Reviewed & tested by the community » Needs review

I'd like an opportunity to review before this goes in.

Wim Leers’s picture

Wim Leers
Location Ghent 🇧🇪🇪🇺
CreditAttribution: Wim Leers at Acquia commented
Assigned: Wim Leers » Unassigned
Status: Needs review » Reviewed & tested by the community

#2: no-brainer 👍

#4 seemed like a good improvement … but I agree that #9 is the more elegant solution, because the CKEditor 5 module seems to have been using ::getAll() which surprisingly is apparently tightly coupled to a specific admin UI use case 😳

All in all, this is a nice simplification! 🤩 I just wanted a chance to double-check because it sounded pretty scary 😅

P.S.: I can not reproduce the local failures though… I just run the tests using PHPStorm, which generates this command: /usr/local/bin/php /private/var/folders/4l/5sndwsz50tqgxjh2525n10br0000gp/T/ide-phpunit.php --configuration /Users/wim.leers/Work/d8/core/phpunit.xml --filter Drupal\\Tests\\ckeditor5\\Kernel\\ValidatorsTest --test-suffix ValidatorsTest.php /Users/wim.leers/Work/d8/core/modules/ckeditor5/tests/src/Kernel.

  • lauriii committed dc51481 on 9.4.x 
    Issue #3249263 by alexpott, andypost, Wim Leers: CKEditor 5 needs...

  • lauriii committed 266679c on 9.3.x 
    Issue #3249263 by alexpott, andypost, Wim Leers: CKEditor 5 needs...
lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii at Acquia commented
Status: Reviewed & tested by the community » Fixed

Committed dc51481 and pushed to 9.4.x. Discussed with @alexpott and we agreed that this should be backported to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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