Problem/Motivation

SourcePluginBase::count() says:

   * Return a count of available source records, from the cache if appropriate.
   * Returns -1 if the source is not countable.

We should define a constant on MigrateSourceInterface to hold this values for better readability.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3215836

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:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

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

guilhermevp’s picture

Status: Active » Needs review
FileSize
1.78 KB

Sending patch please review.

guilhermevp’s picture

FileSize
1.5 KB

Sorry, didn't notice the automatically added use. Please review.

joachim’s picture

Status: Needs review » Needs work

Thanks for the patch! Looks good, just a couple of tweaks:

  1. +++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
    @@ -17,6 +17,11 @@
    +   * Indicates that the instance is not countable.
    

    I would say 'source' rather than 'instance' here.

  2. +++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
    --- a/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    

    This comment should stay, but use the constant with the fully-qualified interface name instead of -1.

  3. Finally, I think there are other source plugins that return -1. The content entity source in migrate_drupal is one.
imalabya’s picture

Status: Needs work » Needs review
FileSize
3.25 KB
2.91 KB

Added a patch to address review comments on #5

guilhermevp’s picture

Status: Needs review » Reviewed & tested by the community

Awesome, thanks @imalabya! Moving to RTBC!

quietone’s picture

Status: Reviewed & tested by the community » Needs work

Thanks for working on this. Remember to check the Coding standards.

  1. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -85,10 +85,10 @@
    + * count the available source records, but just always return
    + * MigrateSourceInterface::NOT_COUNTABLE instead. The high_water_property defines which field
    + * marks the last imported row of the migration. This will get converted into a SQL condition
    + * that looks like 'n.changed' or 'changed' if no alias.
    

    Needs to be wrapped at 80 columns.

  2. +++ b/core/modules/migrate/src/Plugin/migrate/source/SourcePluginBase.php
    @@ -473,7 +473,7 @@ public function getCurrentIds() {
    +   * Returns MigrateSourceInterface::NOT_COUNTABLE if the source is not countable.
    

    Needs to be wrapped at 80 columns.

guilhermevp’s picture

Status: Needs work » Needs review
FileSize
3.26 KB

Thanks quietone! Here is a patch addressing #8.

Please, review.

larisse’s picture

Status: Needs review » Reviewed & tested by the community

The patch #9 was applied and works ok. The Coding standards was fixed too.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

There are a couple of instances of

$this->assertEquals(-1, $source->count());

core/modules/migrate/tests/src/Unit/MigrateSourceTest.php - I think these should be updated to use the new constant too.

swatichouhan012’s picture

Status: Needs work » Needs review
FileSize
4.56 KB
1.16 KB

Created new patch to Update with new constant in core/modules/migrate/tests/src/Unit/MigrateSourceTest.php.
please review.

joachim’s picture

Status: Needs review » Reviewed & tested by the community

LGTM.
I applied the patch and did a search for '-1' in Migrate module and in all files with filepath matching 'Plugins/migrate' and found nothing that looks like a source count.

alexpott’s picture

Title: add a constant to represent uncountable sources for SourcePluginBase::count() » Add a constant to represent uncountable sources for SourcePluginBase::count()
Status: Reviewed & tested by the community » Fixed

Committed 7de8fc1 and pushed to 9.3.x. Thanks!

  • alexpott committed 7de8fc1 on 9.3.x
    Issue #3215836 by guilhermevp, imalabya, swatichouhan012, AJV009,...

Status: Fixed » Closed (fixed)

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