Problem/Motivation

Making Drupal 10 ( D10 ) compatible and fixing tests. Some of the test fails are just label changes. #2805089: Add test coverage should resolve those.

Steps to reproduce

  1. Set up a site running on Drupal 10
  2. Install the Maxlength module with the patch applied

Remaining tasks

  • ✅ Get maintainer approval for the proposed resolution
  • ✅ Implement resolution
  • ❌ Write test coverage (contact a maintainer if you need help)
  • ✅ Maintainer review via the UI
  • ✅ Maintainer Code Review #1
  • ❌ Maintainer code review #2
  • ❌ Merge into dev branch, with credit to author and participants

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#30 Edit-Basic-page-Test-typing-title-MaxLength-2-0-x-dev-Real-physical-testing-with-Drupal-10-not-counted-ckeditor5.png159.73 KBrajab natshah
#30 Edit-Basic-page-Test-typing-title-MaxLength-2-0-x-dev-Real-physical-testing-with-Drupal-10.png161.55 KBrajab natshah
#30 Create-Basic-page-MaxLength-2-0-x-dev-Real-physical-testing-with-Drupal-9.png168.46 KBrajab natshah
#30 Real-physical-testing-round-for-maxlength-2-0-x-dev--with-d10-and-d9.zip2.55 KBrajab natshah
#29 maxlength-3295052-29.patch4.88 KBjoevagyok
#14 maxlength-3295052-14-interdiff.txt585 bytesberdir
#14 maxlength-3295052-14.patch4.6 KBberdir
#13 maxlength-3295052-13.patch4.6 KBberdir
#11 maxlength-3295052-11.patch5.42 KBberdir
#9 interdiff-4-9.txt422 bytesjasonawant
#9 maxlength-3295052-9.patch5.41 KBjasonawant
#4 maxlength-3295052-4.patch5.06 KBberdir
#2 maxlength-3295052-2.patch4.67 KBberdir

Issue fork maxlength-3295052

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs review
StatusFileSize
new maxlength-3295052-2.patch4.67 KB
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Tests are passing on D10 for me.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new maxlength-3295052-4.patch5.06 KB

Ah, I think a patch needs an explicit composer.json requirement/change to work on D10 DrupalCI.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented

Ah, I didn't notice that it only allows me to add 8.x-1.x tests, I guess because 2.0.x has no tests configured.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented 
+++ b/js/maxlength.js
@@ -24,7 +24,7 @@
     detach: function(context, settings) {
       var $context = $(context);
-      $context.find('.maxlength').removeOnce('data-maxlength').each(function () {
+      $(once.remove('maxlength', $context.find('.maxlength'))).each(function () {

the remove seemed a bit strange, it has a data-maxlength prefix, but the once does not, I think that's a bug, but probably doesn't matter?

jasonawant’s picture

jasonawant
he/him
Primary language English
Location New Orleans, USA
commented
Issue summary: View changes

Thanks for the patches. I updated issue summary with a small change to help improve discoverability...I think...

When I searched the issue queue for "Drupal 10", I did not see this issue. It's a bit odd that the issue title isn't used with issue search.

jasonawant’s picture

jasonawant
he/him
Primary language English
Location New Orleans, USA
commented
Assigned: Unassigned » jasonawant
Status: Needs review » Needs work

This patch does not apply cleanly b/c of changes to MaxlengthJavascriptTest.php

Also, there is another .removeOnce usage here that needs to be replaced.

I agreed that the .removeOnce('data-maxlength') was probably a mistake originally, and doesn't matter.

I'm assigning this to myself for now while I prepare another patch.

jasonawant’s picture

jasonawant
he/him
Primary language English
Location New Orleans, USA
commented
Assigned: jasonawant » Unassigned
Status: Needs work » Needs review
StatusFileSize
new maxlength-3295052-9.patch5.41 KB
new interdiff-4-9.txt422 bytes

The interdiff failed to compare patch changes top MaxlengthJavascriptTest.php.

cedewey’s picture

cedewey
he/him
Primary language English
Location Denver, CO
commented
Issue tags: +code review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new maxlength-3295052-11.patch5.42 KB

Patch still/again didn't apply?

cedewey’s picture

cedewey
he/him
Primary language English
Location Denver, CO
commented
Issue summary: View changes

I tested this manually using a site running the latest Drupal 10 dev branch code and the module is working as expected, with the exception of #3267413: Countdown not working for ckeditor5

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new maxlength-3295052-13.patch4.6 KB

Rerolled, left out a test fix for a conflicting test, that completely changed, so I'm going to assume that has been fixed elsewhere.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
StatusFileSize
new maxlength-3295052-14.patch4.6 KB
new maxlength-3295052-14-interdiff.txt585 bytes

Fixed that syntax error.

cedewey’s picture

cedewey
he/him
Primary language English
Location Denver, CO
commented

Thanks Berdir for updating the patch to work with the testing coverage. I see that it is failing the PHP 8.1 & MySQL 5.7, D10.0 test. I'm less versed in the automated testing side of the project. If you can look into why that is, it would be appreciated. Otherwise I can try and get another maintainer to look at it.

Manual testing is still successful, with the module working as expected.

cedewey credited noorulshameera.

cedewey’s picture

cedewey
he/him
Primary language English
Location Denver, CO
commented

A note to us maintainers that noorulshameera helped out with this general Drupal 10 compatibility effort by reporting and contributing a patch to a deprecated function needing replacement in Drupal 10 #3325960: Replace deprecated alter hook "hook_field_widget_form_alter()" with hook_field_widget_single_element_form_alter(). This is already being done in the patch for this issue, but it would still be good to give her credit for helping.

cedewey’s picture

cedewey
he/him
Primary language English
Location Denver, CO
commented
Status: Needs review » Needs work
berdir’s picture

berdir
Primary language German
Location Switzerland
commented

The tests are failing because they are using classy. Needs to be changed to starterkit_base or another theme. I'll do that later

AaronBauman made their first commit to this issue’s fork.

AaronBauman opened merge request !24

aaronbauman’s picture

aaronbauman
he/him
commented

created MR from #14 so i can require with composer

rajab natshah’s picture

rajab natshah
he/him
commented
Priority: Normal » Major
loopy1492’s picture

loopy1492 commented

I've applied the patch to our drupal 9.5.1 site on php 8.1 using the maxlength dev branch and upgrade status seems to be very happy indeed. We eagerly await a new release.

roaldnel’s picture

roaldnel commented

Thank you for all the work that has been put into this so far! We are also eagerly looking forward to the new release.

rajab natshah’s picture

rajab natshah
he/him
commented
Priority: Major » Critical
Status: Needs work » Needs review

Thank you, for having time to maintain the Max Length module.

Hoping for a soft commit to the 2.0.x-dev, to ease testing with Drupal 10.
A soft tag release ( pre-release ) would help having a real physical testing with Drupal ~10.0 using the composer.

Testing now with git clone --branch '2.0.x' https://git.drupalcode.org/project/maxlength.git
And git apply for MR.diff/Patches

joevagyok made their first commit to this issue’s fork.

joevagyok’s picture

joevagyok commented
Issue summary: View changes

Committed the change for the theme, D10 is passing on CI now.

joevagyok’s picture

joevagyok commented
StatusFileSize
new maxlength-3295052-29.patch4.88 KB

Updated the patch as well with the needed fix in the test files to make D10 CI pass.

rajab natshah’s picture

rajab natshah
he/him
commented
Issue summary: View changes
StatusFileSize
new Real-physical-testing-round-for-maxlength-2-0-x-dev--with-d10-and-d9.zip2.55 KB
new Create-Basic-page-MaxLength-2-0-x-dev-Real-physical-testing-with-Drupal-9.png168.46 KB
new Edit-Basic-page-Test-typing-title-MaxLength-2-0-x-dev-Real-physical-testing-with-Drupal-10.png161.55 KB
new Edit-Basic-page-Test-typing-title-MaxLength-2-0-x-dev-Real-physical-testing-with-Drupal-10-not-counted-ckeditor5.png159.73 KB

Thanks, Adam for the updated patch in #29

Had Real physical testing round for MaxLength 2.0.x-dev with Drupal ~10
also
Had Real physical testing round for MaxLength 2.0.x-dev with Drupal ~9.0
Attached the bash commands for the basic testing round
Real-physical-testing-round-for-maxlength-2-0-x-dev--with-d10-and-d9.zip

Working well with Drupal 9 and Drupal 10

But only with plane text fields and CKEditor 4

Still not with CKEditor 5

Facing the #3267413: Countdown not working for ckeditor5 issue

Would you like to have a soft release ( pre-release )?
Still Drupal 9.5.x new installation with the standard profile will face the issue with CKEditor 5
Drupal 10 will need to require CKEditor 4 deprecated module to work ( Tested ).
I'm with having #3267413: Countdown not working for ckeditor5 fixed first

joevagyok’s picture

joevagyok commented

Thanks for the detailed tests and report. We are looking into the CKEditor 5 compatibility to make it work.

kopeboy’s picture

kopeboy
Primary language Italian
Location Milan
commented

If too much work is still needed for CKEditor 5 compatibility, can't we release this for D10 only on (not long) plain text fields in the meantime?

joevagyok’s picture

joevagyok commented
Status: Needs review » Reviewed & tested by the community

All looks good.

joevagyok’s picture

joevagyok commented
Status: Reviewed & tested by the community » Fixed

Merged into 2.1.x-dev.

joevagyok’s picture

joevagyok commented
Version: 2.0.x-dev » 2.1.x-dev
renukakulkarni’s picture

renukakulkarni commented

@Maintainers - When the fix can be available in the non-dev release?

Thanks

joevagyok’s picture

joevagyok commented
Related issues: +#3267413: Countdown not working for ckeditor5

There is the CKEditor 5 related issue that is important to support WYSIWYGs with CKEditor 5 in D10.

smustgrave’s picture

smustgrave commented

Wonder if there could be a beta release of 2.1.x

joevagyok’s picture

joevagyok commented

Yes, I will discuss it with the maintainers this week.

Status: Fixed » Closed (fixed)

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