Problem/Motivation

... and the upstream already has a fixed for since a couple of years: https://github.com/doctrine/annotations/issues/141.

Steps to reproduce

(see minimal test in the MR)

Proposed resolution

Apply fix from upstream.

Remaining tasks

User interface changes

API changes

None

Data model changes

Release notes snippet

Issue fork drupal-3440170

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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

mxr576 created an issue. See original summary.

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 as a volunteer and at Pronovix commented
Status: Active » Needs review

mxr576 opened merge request !7446

larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
CreditAttribution: larowlan at PreviousNext commented

Even though this is on the way out, we will need to support it through the 11.x lifecycle, so there's no harm in improving it in my opinion

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

Ran test-only feature

1) Drupal\Tests\Component\Annotation\Doctrine\DocParserTest::testSupportClassConstants with data set #1 ('@AnnotationWithConstants(\Sim...class)', 'SimpleXMLElement')
Failed asserting that two strings are equal.
--- Expected
+++ Actual
@@ @@
-'SimpleXMLElement'
+'\SimpleXMLElement'
/builds/issue/drupal-3440170/vendor/phpunit/phpunit/src/Framework/Constraint/Equality/IsEqual.php:94
/builds/issue/drupal-3440170/core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php:881
/builds/issue/drupal-3440170/vendor/phpunit/phpunit/src/Framework/TestResult.php:728
/builds/issue/drupal-3440170/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3440170/vendor/phpunit/phpunit/src/Framework/TestSuite.php:684
/builds/issue/drupal-3440170/vendor/phpunit/phpunit/src/TextUI/TestRunner.php:651
/builds/issue/drupal-3440170/vendor/phpunit/phpunit/src/TextUI/Command.php:144
/builds/issue/drupal-3440170/vendor/phpunit/phpunit/src/TextUI/Command.php:97
FAILURES!
Tests: 227, Assertions: 481, Failures: 1.

So coverage for change is there.

Left a comment a comment though, that just may not be me understanding

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

I see other examples of the space in the file. Don't fully understand but does seem to address the issue.

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented
Status: Reviewed & tested by the community » Needs work

PHPCS is disabled here because this is a copy of Doctrine code with different standards, but we should still conform to Doctrine formatting by using 4-space indents.

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

quietone’s picture

quietone CreditAttribution: quietone at PreviousNext commented
Status: Needs work » Needs review
Issue tags: -doctrine, -annotations

I updated the MR to use 4 space indenting on all the changes.

In the future, I would like to have a situation where we don't spend time on formatting issues on forked code that is on the way out.

smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented
Status: Needs review » Reviewed & tested by the community

Rebased for the failing kernel failures. Believe the javascript to be unrelated.

alexpott’s picture

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

Don't particularly like the code... str_starts_with() exists in our minimum PHP but it's a copy of upstream so whatevs... let's march forward to the time we don't have to maintain this.

Committed and pushed f1f27ff7f3 to 11.x and 3adb7e2297 to 10.3.x. Thanks!
Committed 5d783fb and pushed to 10.2.x. Thanks!

  • alexpott committed 5d783fbd on 10.2.x 
    Issue #3440170 by mxr576, quietone, smustgrave, longwave, larowlan:...

  • alexpott committed 3adb7e22 on 10.3.x 
    Issue #3440170 by mxr576, quietone, smustgrave, longwave, larowlan:...

  • alexpott committed f1f27ff7 on 11.x 
    Issue #3440170 by mxr576, quietone, smustgrave, longwave, larowlan:...
smustgrave’s picture

smustgrave CreditAttribution: smustgrave at Mobomo commented

Out of my own curiosity does ! having a space mean anything?

longwave’s picture

longwave
he/him
Primary language English
Location UK
CreditAttribution: longwave at Full Fat Things commented

No - ! $something and !$something mean the same thing, it's just stylistic preference.

quietone closed merge request !7446

mxr576’s picture

mxr576
Primary language Hungarian
Location Hungary
CreditAttribution: mxr576 as a volunteer and at Pronovix commented

Thanks for such a fast resolution of the problem! \o/

let's march forward to the time we don't have to maintain this.

+1