Problem/Motivation

Followup to #3178037: Fix typo "is has" in SimpleTest deprecation warning. There are several places in our code documentation (and one test assertion message) with the typo "is has".

[ayrton:drupal | Tue 14:08:07] $ grep -r " is has " * | grep -v "vendor" | grep -v "node_modules"
core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php:    $this->assertTrue($depth < $depth2, 'Transaction depth is has increased with new transaction.');
core/modules/content_translation/tests/src/Functional/Views/TranslationLinkTest.php:    // @todo Use entity_type once it is has multilingual Views integration.
core/modules/simpletest/simpletest.install:      'description' => t('SimpleTest is has been removed from Drupal 9.0.0 and can no longer be installed. A contributed module is available for those who wish to continue using SimpleTest during the transition from Drupal 8 to 9. <a href=":change-record">See the change record for more information</a>.', [
core/themes/claro/css/theme/media-library.pcss.css: * The added media container receives screen reader focus since it is has the
core/themes/claro/css/theme/media-library.css: * The added media container receives screen reader focus since it is has the
core/themes/seven/css/theme/media-library.css: * The added media container receives screen reader focus since it is has the

Steps to reproduce

grep -r " is has " * | grep -v "vendor" | grep -v "node_modules"

Proposed resolution

Fix the typos (other than the instance in simpletest.install which is a string change and only applicable to 9.1.x.) That one will be handled in #3178037: Fix typo "is has" in SimpleTest deprecation warning.

Remaining tasks

TBD

CommentFileSizeAuthor
#14 3178039-14.patch3.66 KBanmolgoyal74
#8 interdiff_4_8.txt2 KBanmolgoyal74
#8 3178039-8.patch3.67 KBanmolgoyal74
#4 3178039-4.patch3.25 KBMatroskeen
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm created an issue. See original summary.

xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Category: Task » Bug report
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Title: Remove typo "is has" in a few code comments and tests in core » Correct typo "is has" in a few code comments and tests in core
Matroskeen’s picture

Matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
CreditAttribution: Matroskeen as a volunteer and at DevBranch, Drupal Ukraine Community commented
Status: Active » Needs review
FileSize
3178039-4.patch3.25 KB

Here is the patch for 9.2.x branch.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 for Hook 42 commented
Version: 8.9.x-dev » 9.2.x-dev
Issue tags: +Bug Smash Initiative

Thanks for the patch.

1.Patch looks okay for 9.2 and addresses the items from the above list except the simpletest one which was split out into separate issue.

2. A different patch is needed for any backporting to 8.9, if allowed.

3. Tests pass for 9.2.

4. Title and issue summary are clear.

5. I'm not sure why the issue version is set to 8.9 so I'm changing to 9.2.

6. Leaving as needs review for now in case we should manually test any of these.

Abhijith S’s picture

Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commented

Applied patch #4 on 9.2.x . It removes "is has" from 5 files .The simpletest one is already fixed in that issue separately . 

Checking patch core/modules/content_translation/tests/src/Functional/Views/TranslationLinkTest.php...
Checking patch core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php...
Checking patch core/themes/claro/css/theme/media-library.css...
Checking patch core/themes/claro/css/theme/media-library.pcss.css...
Checking patch core/themes/seven/css/theme/media-library.css...
Applied patch core/modules/content_translation/tests/src/Functional/Views/TranslationLinkTest.php cleanly.
Applied patch core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php cleanly.
Applied patch core/themes/claro/css/theme/media-library.css cleanly.
Applied patch core/themes/claro/css/theme/media-library.pcss.css cleanly.
Applied patch core/themes/seven/css/theme/media-library.css cleanly.

RTBC

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
CreditAttribution: alvar0hurtad0 at Factorial GmbH commented
Status: Needs review » Needs work 
  1. +++ b/core/themes/claro/css/theme/media-library.css
@@ -666,7 +666,7 @@
+ * The added media container receives screen reader focus since it has the

    This multi-line comment should suit to coding standard. 

  2. +++ b/core/themes/claro/css/theme/media-library.pcss.css
@@ -616,7 +616,7 @@
- * The added media container receives screen reader focus since it is has the

    This multi-line comment should suit to coding standard. 

  3. +++ b/core/themes/seven/css/theme/media-library.css
@@ -632,7 +632,7 @@
- * The added media container receives screen reader focus since it is has the

    This multi-line comment should suit to coding standard.

anmolgoyal74’s picture

anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commented
FileSize
3178039-8.patch3.67 KB
interdiff_4_8.txt2 KB

Addressed #7

anmolgoyal74’s picture

anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commented
Status: Needs work » Needs review
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Version: 9.2.x-dev » 8.9.x-dev

@Kristen Pol, the version was set to 8.9.x because improvements to tests and code comments are backportable to the bugfix branch, and these same issues exist on 8.9.x as well:

core/tests/Drupal/KernelTests/Core/Database/TransactionTest.php:    $this->assertTrue($depth < $depth2, 'Transaction depth is has increased with new transaction.');
core/modules/content_translation/tests/src/Functional/Views/TranslationLinkTest.php:    // @todo Use entity_type once it is has multilingual Views integration.
core/themes/claro/css/theme/media-library.pcss.css: * The added media container receives screen reader focus since it is has the
core/themes/claro/css/theme/media-library.css: * The added media container receives screen reader focus since it is has the
core/themes/seven/css/theme/media-library.css: * The added media container receives screen reader focus since it is has the

We set the branch to the lowest branch that can receive the branch. If the same patch does not apply to all branches, each patch can have the branch it should be tested on specified at upload.

Thanks!

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 for Hook 42 commented

Thanks for the clarification. From what I recall, I had only see issues start at highest version and then get backported. I'll try to remember this for future issues.

Note that I added 8.9 to the tests so it would show the fails to apply again so it's clear it needs a different patch.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 for Hook 42 commented

@alvar0hurtad0 Thanks for catching that "role" could pop up one line.

@anmolgoyal74 Thanks for the new patch.

The interdiff from #8 addresses the comments from #7.

When trying to patch this against 8.9, the only part that fails is TranslationLinkTest.php because the function signature changed to include the return type:

protected function setUp() {

to

protected function setUp(): void {

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 for Hook 42 commented
Status: Needs review » Needs work

Since #8 looks good for all Drupal 9 branches, moving this back to "needs work" for someone to create a patch for 8.9. Note comment #12 above for why the current patch doesn't apply cleanly to 8.9. Should be a quick fix. Only choose 8.9 for testing that patch. Thanks!

anmolgoyal74’s picture

anmolgoyal74 CreditAttribution: anmolgoyal74 at OpenSense Labs for DrupalFit commented
Status: Needs work » Needs review
FileSize
3178039-14.patch3.66 KB

Re-rolled for 8.9.x

alvar0hurtad0’s picture

alvar0hurtad0
he/him
Primary language Spanish
Location Cáceres
CreditAttribution: alvar0hurtad0 at Factorial GmbH commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott at Acro Commerce, Thunder commented
Status: Reviewed & tested by the community » Fixed

Committed and pushed d8a3cf98f5 to 9.2.x and 0a10997a97 to 9.1.x and 3582dc24fd to 9.0.x. Thanks!

Committed 0db4c8f and pushed to 8.9.x. Thanks!

+++ b/core/themes/claro/css/theme/media-library.css
@@ -658,9 +658,9 @@
+ * The added media container receives screen reader focus since it has the role
+ * 'listitem'. Since it is not an interactive element, it does not need an
+ *  outline.

+++ b/core/themes/claro/css/theme/media-library.pcss.css
@@ -609,9 +609,9 @@
+ * The added media container receives screen reader focus since it has the role
+ * 'listitem'. Since it is not an interactive element, it does not need an
+ * outline.

the css build was not actually run it looks like the .css was edited manually... if there is .pcss.css file then changes should be made there and yarn run build should be run.

Fixed on commit.

  • alexpott committed d8a3cf9 on 9.2.x 
    Issue #3178039 by anmolgoyal74, Matroskeen, xjm, Kristen Pol,...

  • alexpott committed 0a10997 on 9.1.x 
    Issue #3178039 by anmolgoyal74, Matroskeen, xjm, Kristen Pol,...

  • alexpott committed 3582dc2 on 9.0.x 
    Issue #3178039 by anmolgoyal74, Matroskeen, xjm, Kristen Pol,...

  • alexpott committed 0db4c8f on 8.9.x 
    Issue #3178039 by anmolgoyal74, Matroskeen, xjm, Kristen Pol,...

Status: Fixed » Closed (fixed)

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