🇬🇧United Kingdom @oily

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

Improved the issue summary, removing the tag. Pipeline is running all green.

Set to needs review. Reviewer may be able to suggest how to make the test-only test fail without the Symfony mailer module installed.

Change to 'needs review', hope to also get advice on test coverage.

Manual testing in D11 without the fix: screenshot of html entities in email →

I tried to reproduce as in #65 without success. I agree with #63.

@duaelfr Regarding the deprecation test: https://www.drupal.org/about/core/policies/core-change-policies/how-to-d... → . You can search recursively through the core folder for the string '@Group legacy' to find existing deprecation tests.

Pipeline all green.

There is one failing FJ test in PerformanceTest.php. Seems unrelated to this issue. It is the result of a rebase I did.

Changing to Needs Review.

I applied the suggestion of @catch at #12. The new branch and MR are visible above. Not yet sure why but the FunctionalJavascript tests keep passing in the pipeline. But locally in DDEV the test fails and the ouput seems correct See screenshot →

@quietone Thank you for the code suggestions.

Based on recent experience of the WSOD message in the issue summary, these are steps to reproduce:

1. Install Drupal 11.0.x using the standard profile.
2. Use the composer 'drupal/core-recommended' version of drupal
3. If using DDEV, follow the DDEV quickstart for Drupal 11 but specify version 11.0.x
4. Navigate inside the admin theme (logged in as admin) and also logged out
5. Update Drupal to 11.1.x. by following the steps in the drupal composer update → documentation
6. Navigate inside the admin theme (logged in as admin) and also logged out. See if the error appears.
7. Update the database using drush updb
8. Navigate inside the admin theme (logged in as admin) and logged out.

@catch Thank you. That sound like the way ahead..

@dunx Thank you for the details. I am not sure who maintains the code for drupal-cms if its Acquia or Drupal.org. But either way the apparent bugs would surely interest the maintainers.

I did not see anything wrong with the front-end but then I did not experience RC1. The 'Olivero for Drupal CMS' is a child of the Olivero theme. You have to look in the theme .info.yml file to find it out.

It is a little unclear but it looks like you used drush updb. Do you recall whether the error message in the issue summary occurred after you ran that command?

@shalina_jha Yes i noticed that you have stuck closely to the requirements of the parent issue. Looks good. I am not sure if using ->find('css', [some class or id]) loops through each of the tabs. If so then maybe it finds if any of the tabs have the 'bad text' repeated in the title. It might also just find the first element that matches and apply the assert to that? But then ignore the other tabs. If it checks all the matching elements and applies the assert to all of them then that is not good in my opinion.

If for any reason the 'bad text' appeared in all except one of the tabs then that is something we would want the test to find and fail on.

@dries From #47 I note you use claro for admin theme of your site. And a theme 'dries'. Is 'dries' a child theme? If so, what is the parent theme?

@dunx Thank you for the details on your attempts to reproduce the issue. I followed your steps to reproduce in #53 and #55. That is, I followed the steps at https://new.drupal.org/drupal-cms/release-candidate. Running the launch script did not work for me. I had to examine the script and change --project-type=drupal11 to --project-type=drupal

Then the install worked and I could not reproduce the error.

However after reading the related issue https://www.drupal.org/project/drupal/issues/3457863 🐛 YAML discovery does not take theme inheritance into account Active I checked out the 'set to default' themes in the site. Both the admin and the normal themes are child themes eg the admin theme has claro as its parent. Reading through issue 3457863 eg #18 by @pdureau it seems that that issue fixed the handling of parent-child themes.

Given this is a 'major' bug it is worth reaching out on Slack to those involved in the related issue. It may be a case of improving the test coverage in 3457863 so it covers edge cases. And improving the handling of related themes.

@shalini_jha I have made slight changes making the code a bit simpler and more specific. Ready for review.

@shalini_jha I think adding asserts to the test to check that if there are > 1 vertical tabs that the text is repeated x times where x is the number of tabs: that will be fine.

@dunx Hmm, DDEV is involved again. But is that not a vanilla installation? You weren't updating Drupal versions?

@prudloff Ah thanks! In that case I think we can broaden the scope of this issue a bit. The existing issue summary does provide before and after code which shows that a test for HTML entities in the body of the reset email would cover the issue. But the 'trigger' for the issue seems to have changed. It is not triggered by the link in the email body any more.

Now it is more of an edge case. As you mention, it is triggered by the single quotation character. So I think the way ahead is to change the test so it generates a test email with a body containing a quotation mark. Would be worth including a few other similar characters like double quotation marks, things that the exiting fix in the MR will convert from HTML entities back to the plain text versions.

Changing status to 'Needs work'.

Setting issue status to 'Needs review'. Unless the test is deemed useful as a guard against future problems with email as described in the issue summary I think the reviewer may consider postponing this issue for further information or closing it since it appears to be already fixed in core.

@prudloff Can you please specify which version of Drupal and Symfony mailer you were using in #5. If possible can you please provide screenshots to compare with my own.

It seems there is no problem with the password reset email text in current Drupal with default email setup. The problem is when symfony_mailer module (as mentioned in the issue summary) is installed and configured for email. This needs to be made clear in the 'steps to reproduce'.

@shalina_jha I think test coverage should be failing. Since the fix is in the related issue's (3493182) MR/ fork. So this test is running against the existing code which will contain the bug.

Test coverage is in place, removing tag.

The test-only test is passing. This suggests that there are no HTML entities contained in the reset password email. The issue description states,

Before the update to Drupal 10..

That implies that the issue is fixed in Drupal 11.x..

Started work on test coverage. Pushed to the MR.

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

Based on #47 steps to reproduce could be:

1. Install Drupal 11.0.x using the standard profile.
2. Use the composer 'drupal/core-recommended' version of drupal
3. If using DDEV, follow the DDEV quickstart for Drupal 11.
4. Create a skeleton theme.
5. Set the theme as the default non-admin theme.
6. Update Drupal to 11.1.x. by following the steps in the drupal composer update → documentation
7. Update the database using drush

@dries I suggested point 4 in #49 because I tried to reproduce this issue some weeks back using DDEV. I discovered DDEV relies on a particular composer drupal project. Seems you cannot use DDEV with the other major drupal composer project. However all the documentation for doing upgrades I found on drupal.org specify the other major drupal composer project.

I wanted to follow the upgrade steps documented on drupal.org but realised I would probably need to use another tool, perhaps docker4drupal.

So the 'steps to reproduce' will ideally specify the docker tool used and the drupal composer project and the steps taken to upgrade: these vary I think between the 2x drupal composer projects?..

Suggestions:

1. Use a skeleton custom theme in the 'Steps to reproduce': to prove that this is a bug affecting all, not certain themes
2. Still not sure on the next 'steps to reproduce'. Make the skeleton theme the default theme? disable or delete other themes before upgrading?..
3. If the bug can be fixed with several drush commands is it still a bug? Or is that what drush if for?

@dcam Great!

Some fields in Core override the default maxlength attribute up to 1024 characters. There is no reason to not do so in this case.

Yes, but these fields are using a different field widget. I think that a new field widget needs to be created and associated with the core textfield. The link-url form field would then use textfield and the new widget.

@nicxvan Yes, I am reading that with interest.

Okay I will remove it. I have not reproduced the issue yet. So long as it can be reproduced using the steps it should be okay.

The test is failing because there are in fact classes in the page source named 'messages' which have nothing to do with errors.

But the increased maxlength property does not prevent link text > 128 characters from being chopped off at 128. I also tried adding a size property and setting that to > 128 characters. But that has no effect on the textbox either.

Thank you for the code comments @nicxvan. I have responded. I want to be sure that I understand your suggestions.

After removing ignore rule from phpstan baseline, pipeline is running all green.

Ah, thanks, that makes sense.

After acting on code feedback still getting PHPSTAN error so pipeline not green.

Not sure if the PHPSTAN baseline config files deletions in the MR should be reverted.

Okay.

After reading @nicxvan's comments and code feedback I removed the final keyword for affected classes. This seems uncontroversial and so safe to do: it will not affect functionality. Also it has been agreed in policy documents.

@plopesc Thanks.

Created a change record.

@smustgrave Will compute 'yes maybe' == 'yes' : )

@smustgrave Thanks for the feedback. I have added test coverage for when fallback option is empty. Not sure if there are any other logical possiblities?

The pipeline is all green.

Change to 'Needs review'.

Needs before and after screenshots

Rebased MR

oily → changed the visibility of the branch 3389478-warning-undefined-array-11.x to active.

oily → changed the visibility of the branch 3389478-warning-undefined-array to hidden.

oily → changed the visibility of the branch 10.1.x to hidden.

oily → changed the visibility of the branch 11.x to hidden.

oily → changed the visibility of the branch 3389478-warning-undefined-array to active.

oily → changed the visibility of the branch 10.1.x to active.

oily → changed the visibility of the branch 3389478-warning-undefined-array-11.x to hidden.

oily → changed the visibility of the branch 3389478-warning-undefined-array-11.x to active.

oily → changed the visibility of the branch 10.1.x to hidden.

oily → changed the visibility of the branch 10.1.x to active.

oily → changed the visibility of the branch 3389478-warning-undefined-array-11.x to hidden.

#34 I think should be created as a child issue. There is still considerable work required to complete this issue in itself.

The test-only feature is now working correctly.

Removing the Needs tests tag.

There is now a failing test in place. An error message seems to be getting generated when a 'long' url is used.

So it seems that the test coverage is sound. But the fix needs more work. Manual testing should reveal the wording of the error that is being generated.

I agree with #77 that we need steps to reproduce so anybody who knows basic site building or greater can reproduce and test the issue.

#16 delivers a set of steps to reproduce:

Received this error when creating new custom content with an image field.
At the "Manage form display" of the content entity, in the Image field settings, the "Preview image style" was set to "no preview".
Selecting there a predefined image style, e.g. "Thumbnail 100x100" solved the issue for me.
The error disappeared and the edit form of the content type was properly shown.

How does this sound?:

1. Install Drupal 11 using the standard profile and the default theme.
2. Add an image field to the Basic page content type.
3. At the "Manage form display" of the content entity, in the Image field settings, set the "Preview image style" to "no preview".

Rebased.

There are still code comments to be resolved in the MR especially in the tests. The issue tags show other tasks to be completed.

Edited the issue summary. Simplified the description and steps to reproduce and remove duplicate sections.

oily → changed the visibility of the branch 11.x to hidden.

@dcam Great work. This is a long-standing bug!

Not sure if this needs a change record..

oily → changed the visibility of the branch 3258434-increase-linkurl-field to hidden.

@kim.pepper Do you think this requires a change request? This being a pure refactor? e.g. module developers will be unaffected since they will continue to use the existing documented hooks without being concerned with the code is behind them?