Problem/Motivation

API page: https://api.drupal.org/api/drupal/core%21modules%21migrate%21src%21Migra...

MigrateExecutable::import() returns a value, but the documentation for MigrateExecutableInterface::import() doesn't say what it is.

Proposed resolution

Add the documentation

Remaining tasks

Patch
Review
Commit

CommentFileSizeAuthor
#2 3222844-2.patch557 bytesguilhermevp
#5 interdiff_2-5.txt655 bytesdhirendra.mishra
#5 3222844-5.patch624 bytesdhirendra.mishra

Issue fork drupal-3222844

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

joachim created an issue. See original summary.

guilhermevp’s picture

guilhermevp commented
Status: Active » Needs review
StatusFileSize
new 3222844-2.patch557 bytes

Sending patch, please review.

joachim’s picture

joachim commented
Status: Needs review » Needs work

Thanks for the patch!

Though instead of giving the numbers 1 and 4, the docs should use the constants, MigrationInterface::RESULT_FAILED and MigrationInterface::RESULT_COMPLETED.

It also looks like the other MigrationInterface::RESULT_FOO constants can be returned by this line:

      $return = $this->migration
        ->getInterruptionResult();

so they should all be documented.

It might be an idea to use a bullet list for them all for readability.

guilhermevp’s picture

guilhermevp commented
Assigned: Unassigned » guilhermevp
dhirendra.mishra’s picture

dhirendra.mishra commented
Assigned: guilhermevp » Unassigned
Status: Needs work » Needs review
StatusFileSize
new interdiff_2-5.txt655 bytes
new 3222844-5.patch624 bytes

Here is the further correction.

joachim’s picture

joachim commented
Status: Needs review » Needs work

The other MigrationInterface::RESULT_FOO constants need to be listed as I think they are also possible return values.

guilhermevp opened merge request !927

guilhermevp’s picture

guilhermevp commented
Status: Needs work » Needs review

Created MR, please review.

guilhermevp’s picture

guilhermevp commented

Error fixed, please review!

quietone’s picture

quietone commented
Status: Needs review » Needs work
Issue tags: +Bug Smash Initiative
quietone’s picture

quietone commented
Issue tags: +migrate
quietone’s picture

quietone commented
Title: MigrateExecutableInterface::import() doesn't document its return value » Add documentation for the return value of MigrateExecutableInterface::import()
Issue summary: View changes
Issue tags: +Novice

This is a suitable issue for a first time contributor, tagging novice.

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

paulocs’s picture

paulocs
Primary language Portuguese, Brazil
Location Belo Horizonte
commented
Status: Needs work » Needs review
larowlan’s picture

larowlan
Location 🇦🇺🏝.au GMT+10
commented

Hiding patches now there is an MR and crediting Joachim and quietone for mentoring/reviews

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Reviewed & tested by the community

Pointing to the docs where the constants are defined seems better, as then we don't have to keep them in sync if we change anything.

  • catch committed faa874b on 9.3.x 
    Issue #3222844 by guilhermevp, paulocs, dhirendra.mishra, joachim,...
catch’s picture

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

Committed faa874b and pushed to 9.3.x. Thanks!

Status: Fixed » Closed (fixed)

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