The core pipeline is more strict than our contrib pipeline. Upon opening the MR to core, a number of code quality issues were flagged;

https://git.drupalcode.org/issue/drupal-3438895/-/pipelines/144932

The task here is to address the more strict standards by applying the fixes to the contrib module space, and then porting those changes over to the core inclusion MR.

Issue fork navigation-3440498

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

m4olivei created an issue. See original summary.

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

plopesc opened merge request !244

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

Worked on issues related to CSpell, PHPCS & PHPStan.

Some notes:

We should add to the core MR modules/navigation/assets to .cspell.json & .eslintignore ignore list

We should add exceptions for CategorizingPluginManagerTrait in .phpstan-baseline.php file

$ignoreErrors[] = [
	'message' => '#^Call to method getDefinitions\\(\\) on an unknown class Drupal\\\\Core\\\\Plugin\\\\CategorizingPluginManagerTrait\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/lib/Drupal/Navigation/NavigationBlockManager.php',
];
$ignoreErrors[] = [
	'message' => '#^Call to method getSortedDefinitions\\(\\) on an unknown class Drupal\\\\Core\\\\Plugin\\\\CategorizingPluginManagerTrait\\.$#',
	'count' => 1,
	'path' => __DIR__ . '/lib/Drupal/Navigation/NavigationBlockManager.php',
];
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

I added OPT_IN_TEST_NEXT_MAJOR: '1' to the Gitlab CI configuration.

With that, the test suite runs against Drupal 11.x core and the error detailed in #4 no longer shows up. However, another error appears:

There was 1 failure:
1) Drupal\Tests\navigation\Functional\ShortcutsNavigationBlockTest::testNavigationBlock
Failed asserting that two arrays are identical.

This is checking cache tgs on the page, and there is a new unexpected cache tag returning. However we do have a comment there noting that we were anticipating this.

I think the contrib module needs to remain usable on the current release of Drupal (both for developers working on it and test run). I think for these test fixes, it's a case of leaving in the contrib module what satisfies the test runs on the current major, and addressing issues in the next major as a change required in #3439747: [No commit] Prepare for core inclusion, in the MR meant to stand in as a patch for the sync script to apply.

I'll wire that up, and I'll push an update to the core inclusion PR.

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

I'm going to merge what we have here so far so that it can make its way to the core inclusion MR.

The things that are warning at the moment in our contrib CI is eslint, which is a known issue. Also phpstan and phpunit for the next major, which is expected per the conversation here, and I think we can deal with for now.

I'll leave this issue open in case there is more to address after the core inclusion MR is updated with the latest progress from here and from #3439747: [No commit] Prepare for core inclusion.

  • m4olivei committed b23546c4 on 1.x authored by plopesc 
    Issue #3440498 by m4olivei, plopesc: Address code quality issues flagged...
m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented

Nearly there on the latest run!

https://git.drupalcode.org/issue/drupal-3438895/-/pipelines/145241

Just the "Validatable config" is showing a warning, but it's on to the test runs at time of writing 🤞. I'm out of race track for now though.

plopesc’s picture

plopesc
he/him
Primary language Spanish
commented

I think the contrib module needs to remain usable on the current release of Drupal (both for developers working on it and test run). I think for these test fixes, it's a case of leaving in the contrib module what satisfies the test runs on the current major, and addressing issues in the next major as a change required in #3439747: [No commit] Prepare for core inclusion, in the MR meant to stand in as a patch for the sync script to apply.

Good point!

m4olivei’s picture

m4olivei
he/him
Primary language English
Location Grimsby, ON
commented
Status: Active » Fixed

We're green now on the core MR!

There is one warning as previously mentioned, but no errors. That warning is out of our control for the moment.

I'm going to call this one done.

Status: Fixed » Closed (fixed)

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