Problem/Motivation

I found two incorrect type declarations in migrate module:

1. Class property $idMap in \Drupal\migrate\Plugin\Migration declared as string while correct type is array.
2. Class property $result in \Drupal\migrate\Plugin\migrate\id_map\Sql declared as null while correct type is \Drupal\Core\Database\StatementInterface|null

Steps to reproduce

none

Proposed resolution

Fix type declarations.

Remaining tasks

Create merge request.

User interface changes

none

API changes

none

Data model changes

none

Release notes snippet

Issue fork drupal-3212891

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

vaish created an issue. See original summary.

vaish opened merge request !648

vaish’s picture

vaish CreditAttribution: vaish for Greentech Renewables commented
Status: Active » Needs review
Akhildev.cs’s picture

Akhildev.cs CreditAttribution: Akhildev.cs as a volunteer and at Zyxware Technologies commented

hi,
I tried patch #2 manually and for me, the patch applied successfully.
working fine, Thanks

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

@Akhildev.cs, Welcome to Drupal! I am not sure what you mean when you say that you tried the patch manually. This patch changes documentation, not code, so there is nothing to 'try' here. When patches or Merge Requests are tested by the testbot we know that it applies successfully, there is no need to make a comment about that (it adds noise to the issue). The Drupal community has a wealth of information for Getting started with contributing. You may find Review a patch or merge request of particular interest.
Cheers

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz at Four Kitchens commented
Assigned: vaish » Unassigned
Status: Needs review » Reviewed & tested by the community

This seems fine, though I notice that There is no way to set the value of Migration::idMap to anything but an empty array. I suppose it may be set by subclasses, so I'm inclined to leave it for now.

Simple doc change only, so RTBC

alexpott’s picture

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

I added a review to the MR.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Issue tags: +Noivce

The changes here are suitable for a novice.

Matroskeen’s picture

Matroskeen
Primary language Ukrainian
Location 🇺🇦 Ukraine, Lutsk
CreditAttribution: Matroskeen at DevBranch, Evolving Web, Drupal Ukraine Community commented
Issue tags: -Noivce +Novice

Fixing the tag :)

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

brentg’s picture

brentg
Primary language Dutch
Location Ghent
CreditAttribution: brentg as a volunteer commented
Status: Needs work » Needs review

Updated the MR with the review from Alex

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz at Four Kitchens commented
Status: Needs review » Needs work

Still need to remove the |null from the docblock for $result per @alexpott

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

srilakshmier’s picture

srilakshmier CreditAttribution: srilakshmier at Valuebound for Valuebound commented
Status: Needs work » Needs review

Hi, removed |null from the docblock for @result as per #12. Please review.

Thank you

mikelutz’s picture

mikelutz
He/Him
Primary language English
Location Michigan, USA
CreditAttribution: mikelutz at Four Kitchens commented
Status: Needs review » Reviewed & tested by the community

TY!

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 2aee2c17b3 to 9.4.x and 8182c97eb8 to 9.3.x. Thanks!

  • alexpott committed 2aee2c1 on 9.4.x 
    Issue #3212891 by vaish, brentgees@gmail.com, srilakshmier, mikelutz,...

  • alexpott committed 8182c97 on 9.3.x 
    Issue #3212891 by vaish, brentgees@gmail.com, srilakshmier, mikelutz,...

Status: Fixed » Closed (fixed)

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