- 🇺🇸United States jastraat
This updated patch fixes the functional tests for shortcode.
Since the filter transforms on render, we needed a more robust setup for the functional tests that includes having a content type (using the standard install profile), creating a custom filter format, creating nodes with the test content, and then displaying those and comparing the expected result to the actual result.
A few subtle additional changes:
- Drupal has dropped closing tags in self-closing void elements (e.g. img) since they aren't part of HTML5. In order to get the test for the img tag to pass the test needed to take that change into account. See 📌 Remove all references to "self-closing" void elements in core Needs work
- Drupal has some oddities with how it sometimes adds classes to elements, and under some circumstances when there is a single class, a preceding space is added. The tests take that into account.
- Since we're comparing rendered content, special characters like ampersands render as multiple length text strings instead of single character strings leading to unpredictable results when running the random test. I've limited the character set generation to only alphanumeric characters now.
I also rolled the D10 deprecation fixes from 📌 Automated Drupal 10 compatibility fixes Fixed into this patch.
- 🇺🇸United States jastraat
Trying this once again with #3203585: Patch didn't apply, but it seemed like it was only the info file that it failed on → in mind.
- Status changed to Needs work
over 1 year ago 8:09am 29 March 2023 - Status changed to Needs review
over 1 year ago 12:36pm 29 March 2023 - 🇺🇸United States jastraat
The 2.0.x branch is the current one, so that is the branch this patch was created against which is why the patch does not apply. The 2.0.x branch is not available to test against right now. I've created another issue to make it available so that automated tests are useful.
- 🇺🇸United States jastraat
Removing the core: key from the info files again. Waiting to run tests until 2.0.x branch is available.
- 🇮🇹Italy apaderno Brescia, 🇮🇹
The previous patches were tested for the wrong branch (8.x-1.x instead of 2.0.x). Probably that is why the patches failed to apply.
- 🇺🇸United States jastraat
Could someone else please review this so we can get a CKEditor 5 compatible version out the door? We've been using the patch above in production for a few weeks now.
- Status changed to Needs work
over 1 year ago 10:06am 14 May 2023 - 🇺🇸United States DamienMcKenna NH, USA
Did you test it with ckeditor5, or just make it compatible with CKEditor 4 on Drupal 10? This doesn't include any changes for CKEditor 5 itself, everything seems to be for API compatibility with D10, along with some minor edits that should be reverted, e.g.:
@@ -55,7 +124,7 @@ class ShortcodeTest extends BrowserTestBase { $sets = [ [ 'input' => '[button]Label[/button]', - 'output' => '<a href="' . $this->siteUrl . '" class="button" title="Label"><span>Label</span></a>', + 'output' => '<a href="' . $this->siteUrl . '" class=" button" title="Label"><span>Label</span></a>', 'message' => 'Button shortcode output matches.', ], [
and
@@ -84,22 +155,22 @@ class ShortcodeTest extends BrowserTestBase { $sets = [ [ 'input' => '[clear]<div>Other elements</div>[/clear]', - 'output' => '<div class="clearfix"><div>Other elements</div></div>', + 'output' => '<div class=" clearfix"><div>Other elements</div></div>', 'message' => 'Clear shortcode output matches.', ],
etc
I recommend reviewing reverting these changes and starting over with CKEditor 5 on Drupal 9, so that you can avoid spending time on unnecessary changes.
- 🇺🇸United States jastraat
@DamienMcKenna, if you revert those edits, the tests won't pass. When the HTML is generated, it creates spaces ahead of class names.
- 🇺🇸United States jastraat
We have tested with this patch and Drupal 9.5.x + CKEditor 5. We are using our own custom Shortcode plugins that extend ShortcodeBase and they work with CKEditor 5 and this patch.
We don't use the shortcode_basic_tags submodule typically, but I just enabled it in Drupal 9.5 site using CKEditor5 along with the patch above and [highlight] for example works as expected replacing the shortcode syntax with the span tag.
- Status changed to Needs review
over 1 year ago 9:22pm 15 May 2023 - Status changed to Needs work
over 1 year ago 9:37pm 15 May 2023 - 🇺🇸United States DamienMcKenna NH, USA
In order for this module to be compatible with CKEditor 5 it will need a new plugin; you can't say that it's compatible with CKEditor 5 based upon your site because your use case uses a custom plugin and not the code provided by these patches.
Again, the changes in the patches here amount to Drupal 10 compatibility fixes and a minor number of other changes, there's nothing specific to CKEditor 5 in the patches.
- 🇺🇸United States jastraat
Damien, I tested the shortcode basic tags plugin with CKEditor 5. There are pieces to this that make the module compatible with CKEditor 5 - namely changing the filter type from TYPE_MARKUP_LANGUAGE to TYPE_TRANSFORM_IRREVERSIBLE
Have you tested it with CKEditor5 and it did not work?
- Status changed to Needs review
over 1 year ago 7:08pm 16 May 2023 - 🇺🇸United States jastraat
This is a patch to make the existing Shortcode text filters compatible with CKEditor 5, the module as a whole compatible with Drupal 10, and fix the existing shortcode tests so that they pass.
The existing Shortcode 2.x code does not include a CKEditor 4 plugin (e.g. a class that extends CKEditorPluginBase) so while there may be a desire to add that for CKEditor 5, it would be new functionality. Marking back to needs review.
If someone has tested this patch with CKEditor 5 and are getting errors, please post those errors so we can get them fixed! Thanks :)
- 🇺🇸United States DamienMcKenna NH, USA
My apologies, I mistakenly thought the module did have a CKEditor plugin.
- last update
over 1 year ago Composer require failure - Status changed to Needs work
over 1 year ago 6:13pm 28 June 2023 - 🇺🇸United States mile23 Seattle, WA
Verifying that the patch in #11 still applies and doesn't seem to break anything. :-)
Using a path repo setup in a Drupal core dev environment, I was able to verify that I can install and enable the patched module with core 9.5, 10.0, and 10.1. Also was able to edit text and add shortcodes.
It would be super-nice if the dev branch were passing tests, and we could see changes to tests here.... I don't see another issue to fix those so let's do it here, unless someone wants to file it.
The test changes in the patch do (mostly) fix the tests so they'll run again.
I had passing tests for Drupal 9.5x and 10.0.x.
Using Drupal 10.1.x I only saw one fail:
$ ./vendor/bin/phpunit --config ./core/phpunit.xml --group shortcode PHPUnit 9.6.8 by Sebastian Bergmann and contributors. Testing ........F 9 / 9 (100%) Time: 02:31.670, Memory: 158.00 MB There was 1 failure: 1) Drupal\Tests\shortcode\Functional\ShortcodeTest::testRandomShortcode Random shortcode self-closed, output length is correct. Failed asserting that 6 matches expected 8. /var/www/html/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:96 /var/www/html/shortcode/tests/src/Functional/ShortcodeTest.php:400 /var/www/html/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
- last update
over 1 year ago Composer require failure - last update
over 1 year ago Composer require failure - Status changed to RTBC
over 1 year ago 8:32pm 3 July 2023 - 🇺🇸United States mile23 Seattle, WA
@jastraat mentioned that she couldn't reproduce the fail, so I ran it again and it passed.
It would be *very* much better if a maintainer could turn on automatic CI for this project.... :-)
- 🇮🇹Italy apaderno Brescia, 🇮🇹
Automatic testing was enabled, but it used PHP 7.3 with Drupal 9.5, which requires PHP 7.4. I corrected that; tests for the 2.0.x branch now use PHP 7.4 and Drupal 9.5.
- Open on Drupal.org →Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7last update
over 1 year ago Waiting for branch to pass - 🇮🇹Italy apaderno Brescia, 🇮🇹
With the currently committed code, one of the module tests fails. If that happens for this patch too, the culprit is not the patch.
- 🇺🇸United States jastraat
I believe this code may correct the failing test in the committed code -
- 🇺🇸United States jastraat
@Denes.Szabo pinging to see if you could look at / commit this to add compatibility for shortcode. This is our last module hold out for upgrading to Drupal 10.
-
Denes.Szabo →
committed b5b64fce on 2.0.x authored by
jastraat →
Issue #3280596 by jastraat, adiro, fraserthompson, apaderno,...
-
Denes.Szabo →
committed b5b64fce on 2.0.x authored by
jastraat →
- Status changed to Fixed
over 1 year ago 8:35am 22 July 2023 - 🇭🇺Hungary denes.szabo Hungary
Thank you for your contribution, the 2.0.2 just released.
- Status changed to Fixed
over 1 year ago 8:35am 22 July 2023