Problem/Motivation

PHPStan baseline is currently skipping multiple Call to an undefined method errors in the migration system. Other minor phpstan errors also addressed:

  • Call to an undefined method
  • removed unnecessary usages of CreateTestContentEntitiesTrait
  • ::installEntitySchemas has no return type specified

Proposed resolution

Fix the errors in the following

  • core/modules/migrate/tests/src/Kernel/MigrateTestBase.php
  • core/modules/migrate_drupal/tests/src/Kernel/d6/MigrateDrupal6AuditIdsTest.php
  • core/modules/migrate_drupal/tests/src/Kernel/d7/MigrateDrupal7AuditIdsTest.php
  • core/modules/migrate_drupal/tests/src/Traits/CreateTestContentEntitiesTrait.php
  • core/modules/migrate_drupal_ui/tests/src/Functional/MigrateUpgradeTestBase.php
  • core/modules/migrate_drupal_ui/tests/src/Functional/d7/FilePathTest.php
CommentFileSizeAuthor
#9 3469836-nr-bot.txt90 bytesneeds-review-queue-bot

Issue fork drupal-3469836

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

quietone created an issue. See original summary.

quietone opened merge request !9310

quietone’s picture

quietone commented
Status: Active » Needs review
smustgrave’s picture

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

Seems straight forward and does address the phpstan issues. Wondered if we should add typehints but probably would be a stretch of scope.

quietone’s picture

quietone commented

Rebased, these was a conflict in the phpstan baseline.

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Needs work

Added some comments to the MR. I feel that we shouln't make breaking changes or have extra to maintain just because PHPStan can't work something out.

quietone’s picture

quietone commented
Status: Needs work » Needs review

And in my work on this I also removed unnecessary usages of CreateTestContentEntitiesTrait.

quietone’s picture

quietone commented
Component: phpunit » migration system
needs-review-queue-bot’s picture

needs-review-queue-bot commented
Status: Needs review » Needs work
StatusFileSize
new 3469836-nr-bot.txt90 bytes

The Needs Review Queue Bot tested this issue. It no longer applies to Drupal core. Therefore, this issue status is now "Needs work".

This does not mean that the patch necessarily needs to be re-rolled or the MR rebased. Read the Issue Summary, the issue tags and the latest discussion here to determine what needs to be done.

Consult the Drupal Contributor Guide to find step-by-step guides for working with issues.

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

quietone’s picture

quietone commented
Status: Needs work » Needs review

Rebase and regenerate the baseline.

smustgrave’s picture

smustgrave commented

Tried adding a return type to the new method but got

Return type mixed of method                                                           
         Drupal\Tests\migrate_drupal_ui\Functional\d7\FilePathTest::getManagedFiles()          
         is not covariant with return type array of method                                     
         Drupal\Tests\migrate_drupal_ui\Functional\MigrateUpgradeTestBase::getManagedFiles().  
 ------ --------------------------------------------------------------------------------------
smustgrave’s picture

smustgrave commented

Re-ran unit test failure and was random.

+1 from me but there are 2 open threads from @alexpott what are your thoughts on the responses?

smustgrave’s picture

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

Going to go out on a limb and mark this one. If premature I apologize.

mstrelan’s picture

mstrelan commented
Status: Reviewed & tested by the community » Needs work

Added one small docs thing to fix

binoli lalani made their first commit to this issue’s fork.

quietone’s picture

quietone commented
Status: Needs work » Needs review

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

heddn’s picture

heddn
Primary language English
Location Nicaragua
commented
Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

Made a few minor fixes given the last few asks. This looks pretty good at this point. Given all I did was switch a comment to {@inheritdoc}, I think I can still mark this RTBC.

catch closed merge request !9310

  • catch committed 3640baef on 11.x 
    Issue #3469836 by quietone, smustgrave, heddn, binoli lalani, alexpott,...
catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x, thanks!

Status: Fixed » Closed (fixed)

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