Problem/Motivation

Properties $currentRow and $currentSourceIds in the SourcePluginBase might be NULL. See SourcePluginBase::next() which explicitly sets the values of both properties to NULL. But neither of the docblocks for the properties mention NULL in the @var type hint.

Proposed resolution

Add NULL to the property type hints.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

CommentFileSizeAuthor
#3 core-3530841-3.patch722 bytesmero.s

Issue fork drupal-3530841

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

mero.s created an issue. See original summary.

mero.s’s picture

mero.s commented
Issue summary: View changes
mero.s’s picture

mero.s commented
Assigned: mero.s » Unassigned
Status: Active » Needs review
StatusFileSize
new core-3530841-3.patch722 bytes

Added patch, please review.

smustgrave’s picture

smustgrave commented
Version: 10.4.x-dev » 11.x-dev
Status: Needs review » Needs work
Issue tags: +Needs issue summary update, +Needs steps to reproduce, +Needs tests

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

oily opened merge request !12439

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

dcam’s picture

dcam commented
Title: Property Drupal\migrate\Plugin\migrate\source\SourcePluginBase::$currentRow (Drupal\migrate\Row) does not accept null. » Update SourcePluginBase property type hints
Component: plugin system » migration system
Category: Bug report » Task
Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs issue summary update, -Needs steps to reproduce, -Needs tests

Updates to type hints in documentation are typically classified as tasks, not bug reports. This doesn't need tests or steps to reproduce.

smustgrave’s picture

smustgrave commented
Category: Task » Bug report

Updates to type hints in documentation are typically classified as tasks, not bug reports. This doesn't need tests or steps to reproduce.

That's not actually true wrong docs can be considered a bug.

benjifisher’s picture

benjifisher
he/him or they/them
Primary language English
Location Boston area
commented

Looking at recent children of #3404246: [META] Fix strict type errors detected by phpstan, I see that they are categorized as bug reports, not tasks, so let's stick with bug report for this issue.

I agree with Comment #8 that this issue does not need tests nor steps to reproduce.

The changes in the MR look right to me. My only question is whether it is worth fixing just two @var comments in one file or if this change should be part of a larger issue. Is there a way to get PHPStan to find mismatches like this in bulk?

smustgrave’s picture

smustgrave commented

Mstrelan may know. He’s been doing a ton of work around fixing those

smustgrave’s picture

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

Since this is such a small change lets move it forward.

  • catch committed d8bbc9d7 on 11.x 
    task: #3530841 Update SourcePluginBase property type hints

By: mero.s...

catch closed merge request !12439

catch’s picture

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

It's tempting to try to do this with a rector rule, but my overall feeling is we need to individually review each case. e.g. there could be a different class where the property isn't initialized but should be, to an array or whatever. We could discuss that more on the meta though, maybe there's a way to automate some of it.

Either way committed/pushed this one to 11.x, thanks!

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

Status: Fixed » Closed (fixed)

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