🇯🇵Japan @ptmkenny

ptmkenny → created an issue.

ptmkenny → created an issue.

I did a basic code review. Please correct me if I'm wrong, but I think the changes need to be made on the 3425240-automated-drupal-11-fix branch (or a new branch), but not the project-update-bot-only because that branch is, as the name suggests, only for project-update-bot.

ptmkenny → created an issue.

No, it isn't merged. At present, you need to apply a patch from the MR to use the module.

CKEditor support for vite is still experimental; last year, when I added the editor to a TypeScript project (not a plugin, just getting the standard editor set up), it was a bit of a pain. I think the official build toolchain is all in webpack, so it's probably better to stick with webpack for now.

ptmkenny → created an issue.

ptmkenny → created an issue.

@Anybody Do you have a suggestion for an alternate library?

Hmm, maybe CKEditor requires the plugins not to be exported by default? Just a guess.

what purpose does making ckeditor5 a dependency instead of a dev-dependency serve?

This is just a standard eslint error. Eslint requires anything that is imported in the build to be in dependencies instead of devDependencies. I am not an expert on this, but usually I don't think it matters either way. However, there may be cases where webpack, vite, and other build tools can make mistakes because they will strip devDependencies, so if a real dependency that gets imported is in dev, what should the build tool do, strip it or keep it? To avoid this kind of confusion, eslint says make it a dependency.

So there was one final error I wasn't sure about:

/builds/issue/token_filter-3433142/web/modules/custom/token_filter-3433142/js/plugins/tokenbrowser/plugin.js
  62:20  error  'CKEDITOR' is not defined  no-undef
✖ 1 problem (1 error, 0 warnings)

The code appears to be based off of Entity Embed module's CKEditor plugin, but that has the same eslint issue.

So, I just ignored this line, since CKEDITOR should always be defined, right?

If not, hopefully someone else can fix this.

The only other thing I ignored was the build output, but that should be ignored because build artifacts don't need to be linted.

I did NOT manually test this because I don't use CKEditor on my site, so I would appreciate if someone who is using the plugin can test this and confirm everything still works.

ptmkenny → created an issue.

Ok, here are the remaining issues:

/builds/issue/token_filter-3433142/web/modules/custom/token_filter-3433142/js/ckeditor5_plugins/tokenBrowser/src/token-browser-command.js
  1:25  error  Unable to resolve path to module 'ckeditor5/src/core'  import/no-unresolved
/builds/issue/token_filter-3433142/web/modules/custom/token_filter-3433142/js/ckeditor5_plugins/tokenBrowser/src/token-browser-editing.js
  1:24  error  Unable to resolve path to module 'ckeditor5/src/core'  import/no-unresolved
/builds/issue/token_filter-3433142/web/modules/custom/token_filter-3433142/js/ckeditor5_plugins/tokenBrowser/src/token-browser-ui.js
  1:24  error  Unable to resolve path to module 'ckeditor5/src/core'  import/no-unresolved
  2:28  error  Unable to resolve path to module 'ckeditor5/src/ui'    import/no-unresolved
/builds/issue/token_filter-3433142/web/modules/custom/token_filter-3433142/js/ckeditor5_plugins/tokenBrowser/src/token-browser.js
  1:24  error  Unable to resolve path to module 'ckeditor5/src/core'  import/no-unresolved
/builds/issue/token_filter-3433142/web/modules/custom/token_filter-3433142/js/plugins/tokenbrowser/plugin.js
  62:20  error  'CKEDITOR' is not defined  no-undef
/builds/issue/token_filter-3433142/web/modules/custom/token_filter-3433142/webpack.config.js
  3:25  error  Unable to resolve path to module 'webpack'                import/no-unresolved
  4:30  error  Unable to resolve path to module 'terser-webpack-plugin'  import/no-unresolved
✖ 8 problems (8 errors, 0 warnings)

7 of the issues are a result of ckeditor5/webpack not being installed.

As for how to solve this, in Drupal Slack #gitlab, @fjgarlin stated:

this is what core does https://git.drupalcode.org/project/drupal/-/blob/11.x/.gitlab-ci.yml?ref_type=heads#L359
maybe you need to run that somehow. maybe in the composer:after_script because we run yarn in the composer job: https://git.drupalcode.org/project/gitlab_templates/-/blob/main/includes/include.drupalci.main.yml#L350

or ignore

@darvanen Thanks for the quick merge! Updated and requiring eslint to pass as part of this issue.

Many of the issues are with js/build/tokenBrowser.js, but this file probably doesn't need to be linted because it is already built and not meant to be user-editable.

This MR does three things:

• require composer-lint, cspell, phpcs, and stylelint to pass
• fixes the phpcs test (easy automated fixes)
• updates the gitlab-ci template to the latest version (removes boilerplate-- taken from https://git.drupalcode.org/project/gitlab_templates/-/blob/main/gitlab-c...)

ptmkenny → created an issue.

ptmkenny → made their first commit to this issue’s fork.

Prettier just does some basic automatic formatting; it wants you to add whitespace even though it shows the dots.

These kind of fixes can be annoying to do manually, but you can download the patch generated in the build artifacts for the eslint job and apply that, which will automatically fix the things flagged by prettier in most cases.

Currently we do not provide our own API method for validating tokens, so the validation token form does not throw this exception.

ptmkenny → created an issue.

I installed this on my site and, after disabling the Content Security Policy module, got the basic functionality working.

However, src/JsonFieldRequirements.php is still searching for yesmeck, which was removed from the README. Is jquery-jsonview also supposed to be removed by this MR? If so, JsonFieldRequirements.php needs to be updated, and if not, then the info on jsonview needs to be put back in the README.

Unit tests are failing on Drupal 9:

There were 2 errors:
1) Drupal\Tests\json_field\Kernel\PrettyElementTest::testElement
TypeError: Argument 1 passed to Drupal\json_field\Element\JsonPretty::formatJson() must be an instance of Drupal\json_field\Element\mixed, instance of stdClass given, called in /builds/issue/json_field-3411495/src/Element/JsonPretty.php on line 31
/builds/issue/json_field-3411495/src/Element/JsonPretty.php:40
/builds/issue/json_field-3411495/src/Element/JsonPretty.php:31
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php:101
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:788
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:374
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:204
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:160
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:580
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:161
/builds/issue/json_field-3411495/tests/src/Kernel/PrettyElementTest.php:59
/builds/issue/json_field-3411495/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
2) Drupal\Tests\json_field\Kernel\PrettyFormatterTest::testFormatter
TypeError: Argument 1 passed to Drupal\json_field\Element\JsonPretty::formatJson() must be an instance of Drupal\json_field\Element\mixed, instance of stdClass given, called in /builds/issue/json_field-3411495/src/Element/JsonPretty.php on line 31
/builds/issue/json_field-3411495/src/Element/JsonPretty.php:40
/builds/issue/json_field-3411495/src/Element/JsonPretty.php:31
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Security/DoTrustedCallbackTrait.php:101
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:788
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:374
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:446
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:446
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:204
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:160
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:580
/builds/issue/json_field-3411495/web/core/lib/Drupal/Core/Render/Renderer.php:161
/builds/issue/json_field-3411495/tests/src/Kernel/PrettyFormatterTest.php:36
/builds/issue/json_field-3411495/vendor/phpunit/phpunit/src/Framework/TestResult.php:729
ERRORS!
Tests: 17, Assertions: 96, Errors: 2.

I fixed the issues identified by cspell, phpcs, and stylelint and synced the branch.

Honestly I never really worked with JQuery so I don't understand what's going on here. All my work recently has been in React/TypeScript, so I'm lost about how to fix these issues.

Now that phpstan is fixed, we could require that test to pass as well.

However, phpstan (next minor) is not passing because of the deprecation in 📌 Drupal 12 compatibility Active , so if we require phpstan to pass, then when 10.3 is released, tests will break until that issue is fixed (or we phpstan-ignore it).

ptmkenny → created an issue.

Ok, four problems remain:

  26:35  error  Prefer textContent to $.text  jquery/no-text
/builds/issue/json_field-3450116/web/modules/custom/json_field-3450116/modules/json_field_widget/assets/js/json_widget.js
  44:17  error  Prefer textContent to $.text                 jquery/no-text
  44:32  error  'jsonEditor' was used before it was defined  no-use-before-define
  51:36  error  'JSONEditor' is not defined                  no-undef
✖ 4 problems (4 errors, 0 warnings)

I don't know how to fix these.

ptmkenny → created an issue.

I'm sorry, I misread the issue and didn't realize it was about phpstan. I'll try to read more carefully next time!

ptmkenny → created an issue.

Also require composer-lint to pass since it is passing.

Working on it over here: https://www.drupal.org/project/json_field/issues/3450099 📌 Require phpcs, cspell, and stylelint to pass Active

Added stylelint to the required tests to pass since it is already passing (policy: once a test is passing, require all future commits not to break it).

ptmkenny → created an issue.

Hmm, composer install won't run on GitLab CI because Feeds doesn't yet have a dev branch that supports Drupal 11. As a temporary measure, I'm going to remove feeds as a dependency, which should allow the test to run, but we should add this back before the final release (because it will introduce phpstan failures).

CI is in so I merged in the latest changes and am opening this back up.

Created MR so that this can be tested with GitLab CI. This is the patch in #3 with no changes; all credit should go to Chi.

ptmkenny → made their first commit to this issue’s fork.

Merged in the latest changes so we can check the result on GitLab CI.

Merged in the latest changes so GitLab CI will run to check this.

ptmkenny → created an issue.

ptmkenny → created an issue.

ptmkenny → created an issue.

ptmkenny → created an issue.

@clarkssquared Good catch, the coding standards had been updated since this was originally submitted.

Cspell words can be defined in the GitLab CI template instead of a separate dictionary file, so I moved "validateable" definition to the template added in 📌 Add GitLab CI template Needs review .

Unit tests are broken but that is because of the PHP 8.2 deprecation fixed in 🐛 PHP 8.2 compatibility Needs review .

This is a helpful comment and should be in the docs. RTBC.

Upgrade Status module scan reveals no problems other than bumping the core version in .info.yml.

ptmkenny → created an issue.

ptmkenny → created an issue.

ptmkenny → created an issue.

ptmkenny → created an issue.

ptmkenny → created an issue.