Most recent comment as of this summary update: #34

Problem/Motivation

The migrate module is still considered an experimental module. As a result, its code and tests have been reviewed less than other more mature code.

Proposed resolution

Adhere to coding standards.
Attached are changes noted during a review of the files in core/modules/migrate/src/*.php. These changes include adding docblocks, missing @param/@var/@return descriptions and a few coding style changes.

Remaining tasks

Re-roll patch

According to comment #13 by @xjm, I think this needs more than just a re-roll. It needs to be broken apart into separate issues and each of those issues should address distinct points.

User interface changes

To be determined

API changes

To be determined

Data model changes

To be determined

Comments

Lars Toomre created an issue. See original summary.

Lars Toomre’s picture

Status: Active » Needs review
FileSize
36.9 KB

Attached is an initial patch with changes to sixteen files. The patch was generated with the -U6 switch so that one could more easily see the method signature when type hinting was added.

Lars Toomre’s picture

Created #2624648: False sniff for missing comment of a @throws directive in Coder module to address false sniff condition 'Comment missing or not on the next line for @throws tag in function comment' in Drupal.Commenting.FunctionComment.EmptyThrows.

Lars Toomre’s picture

Added details about false positive in-line type hint sniff to Coder issue #2624076: Support namespace aliases in /** @var */ definitions.

benjy’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Entity/MigrationInterface.php
    @@ -107,26 +107,27 @@
    +   * @param array|null $process
    

    Shouldn't NULL be capitals here?

  2. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -263,13 +265,13 @@ public function import() {
    -      unset($sourceValues, $destinationValues);
    +      unset($this->sourceValues, $this->destinationValues);
    

    This seems like a functional change, we shouldn't do that here?

  3. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -525,18 +528,18 @@ protected function getMemoryUsage() {
    +    // @todo Explore resetting the container.
    

    Again, missing an issue. Maybe we should add follow-ups to find issues or remove the todo

Lars Toomre’s picture

@benjy Thanks for the comments. When you have a chance can you take a look at #2621486: Fixes to migrate/tests/src/Unit/*.php files when you have a moment? I cannot figure out for the life of me why MigrateExecutableTest.php has failed (even when it is not included in the patch)? Thanks.

Regarding 5.1, our documentation standards call for it to be null and not NULL. One can check https://drupal.org/node/1354 for more information.

Regarding 5.2, the coder_sniff highlighted this as an error. If you check the file, you will notice that $sourceValues and $destinationValues do not exists independent of the class properties. I am pretty sure that this is what the code intended originally, but am open to any suggested change.

Regarding 5.3, that @todo was already in the code that was committed. If you see an issue for it, I am happy to modify the patch to include it. With a cursurory review, I did not see such an issue open.

benjy’s picture

Yeah, I was just pointing out that todo's should either have an issue or we should create one since we're touching it :)

heddn’s picture

Title: Fixes to migrate/src/*.php files » Fixes to docs and code style for migrate/src/*.php files
quietone’s picture

@benjy, will #2545632: [PP1] Move memory reclamation out of migrate executable do as an issue for the @TODO?

benjy’s picture

Yeah, that works for me.

Lars Toomre’s picture

Status: Needs work » Needs review
FileSize
832 bytes
37.2 KB

Attached are an interdiff and new patch that incorporates the issue #2545632: [PP1] Move memory reclamation out of migrate executable as the subject of the @todo directive.

quietone’s picture

Status: Needs review » Reviewed & tested by the community

benjy's concerns in #5 have been answered. The final piece was the addition of a link to an issue in the @todo. With that done this can be RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/migrate/src/MigrateExecutable.php
    @@ -263,13 +265,13 @@ public function import() {
    -      unset($sourceValues, $destinationValues);
    +      unset($this->sourceValues, $this->destinationValues);
    

    This functional change implies missing test coverage I don't think it should be in this patch - we should open a separate issue.

  2. +++ b/core/modules/migrate/src/MigrateTemplateStorage.php
    @@ -38,13 +40,23 @@ class MigrateTemplateStorage implements MigrateTemplateStorageInterface {
       /**
    +<<<<<<< HEAD
        * {@inheritdoc}
    +=======
    +   * Finds all migration templates with the specified tag.
    +   *
    +   * @param string $tag
    +   *   The tag to match.
    +   *
    +   * @return array
    +   *   Any templates (parsed YAML config) that matched, keyed by the ID.
    +>>>>>>> Fixes to src/*.php
        */
       public function findTemplatesByTag($tag) {
    
    @@ -54,13 +66,23 @@ public function findTemplatesByTag($tag) {
       /**
    +<<<<<<< HEAD
        * {@inheritdoc}
    +=======
    +   * Retrieves a template given a specific name.
    +   *
    +   * @param string $name
    +   *   A migration template name.
    +   *
    +   * @return NULL|array
    +   *   A parsed migration template, or NULL if it does not exist.
    +>>>>>>> Fixes to src/*.php
        */
       public function getTemplateByName($name) {
         $templates = $this->getAllTemplates();
    

    Messed up patch

Lars Toomre’s picture

Issue #2645164: Fix use of $sourceValues and $destinationValues local variables has been created to address change highlighted in #13.1.

Attached are a patch and interdiff that address the failed re-roll update highlighted in #13.2. A couple of other changes were made as a result of reviewing the resulting patch.

quietone’s picture

Status: Needs review » Reviewed & tested by the community

Apologies for missing the conflict in the patch. A new issue has been created as requested and the patch looks good, so back to RTBC.

alexpott’s picture

Status: Reviewed & tested by the community » Needs work

@Lars Toomre and the reviewers thanks for your work here. Recently the committer team and @xjm especially have put a ton of work into guidelines for how to scope issues. This issue is a grab bag of fixes that makes it difficult to review see https://www.drupal.org/core/scope for examples of how to scope issues. Ideally all the new documentation what be split into a sensibly scoped patches. And the coding standard fixes can be grouped by standard that can be reviewed with phpcs for the whole of core.

Lars Toomre’s picture

Issue tags: +Novice

This issue was stated before those new guidelines were posted. It is a review of a new experimental module from a documentation perspective. As a result, it does contain a number of different types of changes. I am sorry that this work will not be committed as is. Others are welcome to split it up into smaller focused patches that are easier to review. Adding the 'Novice' tag as a result.

xjm’s picture

Thanks @Lars Toomre; I think that could be a good novice task (or set of novice tasks). Here are a few of the types of automatable fixes I see in this patch that can be addressed according to the coding standards cleanup guidelines (which BTW have existed since November 2014; they just weren't a part of this new single handbook page):

  • One-line summaries should be a single line of 80 characters or less.
  • The verb at the start of the one-line summaries for classes and functions should be singular third-person, present tense, indicative -- basically, the first word should end in an "s". (I can't think of any exceptions to this in English and http://learnersdictionary.com/qa/third-person-singular-s-simple-present-... agrees.)
  • Proper indentation of docblock sections.
  • Wrapping of all comments to fewer than 80 characters unless the line contains only a single word that is too long (or one word plus other parts of @see @param @link etc.).
  • Inline comments should have a space after the //.
  • Missing newlines according to various rules.
  • Our friend (optional) again.
  • Wrong parameter name in $param.

So all of those things would be good starting points to follow https://www.drupal.org/core/scope#coding-standards and contribute in #2571965: [meta] Fix coding standards in core (one scope each).

Then there are a few kinds of fixes that involve improving documentation, so it can make sense to fix them for Migrate alone per the note at the end of the coding standards guidelines:

  • Writing the missing documentation (coder can check for the absence or presence only).
  • Other improvements to the documentation itself (for clarity, missing information, miscellaneous incorrect formatting, etc.)
ferhervi’s picture

Continued with the work but not finished it.

tbrameshbabu’s picture

Status: Needs work » Needs review

Switching to Needs Review to test the above patch

deepakaryan1988’s picture

Assigned: Unassigned » deepakaryan1988
deepakaryan1988’s picture

Assigned: deepakaryan1988 » Unassigned

@xjm It looks like good to me.

Please have a review.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

jeffrey.vargas’s picture

I'm working on this in the Mentor Sprints at DrupalCon NOLA.

It no longer applies to 8.2.x-dev and requires a re-roll.

hussainweb’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll

Thanks @jeffrey.vargas. Since this is a bug-report, this should go to 8.1.x (migrate is experimental anyway). I have added the tag which you may remove once you reroll and upload the patch.

jeffrey.vargas’s picture

Thanks hussainweb. Will review against 8.1.x.

kurthill4’s picture

We're a group at a conference, we'll look at it now as part of a sprint.

nJim’s picture

Will add an issue summary to this ticket.

The Sean’s picture

Issue summary: View changes
The Sean’s picture

Issue summary: View changes
The Sean’s picture

Issue summary: View changes
vegantriathlete’s picture

Great! We've got a couple awesome people working on https://www.drupal.org/contributor-tasks/reroll.
They will additionally test out the instructions at: https://www.drupal.org/patch/reroll

vegantriathlete’s picture

We've got another contributor working on: https://www.drupal.org/contributor-tasks/write-issue-summary

The Sean’s picture

Issue summary: View changes

Updating the Issue Summary

The Sean’s picture

Issue summary: View changes

Updating summary again - modifying Proposed Solution. I am new at this.

kurthill4’s picture

Ran into merge conflicts on four files, resolved and documenting... Stay tuned...

nJim’s picture

Ran into conflict during rebase process. Including a description of our resolutions:

/core/modules/migrate/Plugin/Migration.php
Retained the comment to read 'migration manager' to be consisten with the name of the function.
Retained the public function getDestinationIds.

/core/modules/migrate/Plugin/MigrationInterface.php
Retained the comments from the latest drupal release.

/core/modules/migrate/src/MigrateMessageInterface.php
Included the new file doc comment.

/core/modules/migrate/src/MigrateBuildDependencyInterface.php
Included the new file doc comment.

/core/modules/migrate/src/MigrateTemplateStorage.php
Included the new file.

/core/modules/migrate/src/MigrateTemplateStorageInterface.php
Included the new file.

kurthill4’s picture

Previous comment documents merge conflict resolutions

nJim’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 38: 2624640-38-migrate-module-src-d8.patch, failed testing.

kurthill4’s picture

We spoke with one of the mentors who went over the fallure to apply; the issue seems to be related to path changes that occurred between 8.0.x and 8.2.x, when migrate was "moved out of entity". I plan to revisit this, but won't have time, for several days, probably. So if someone else wants to go over our patch and provide any feedback/comments, that would be great! If you take it and fix it, any summary of where we went wrong would also be great!

Thanks!

vegantriathlete’s picture

Issue summary: View changes
vprocessor’s picture

Status: Needs work » Needs review
FileSize
30.54 KB

#19 - rerolled

Mile23’s picture

Note that we're also fixing coding standards systematically by coding standard error here: #2571965: [meta] Fix coding standards in core

mikeryan’s picture

Status: Needs review » Postponed
Issue tags: -Needs reroll

At least some of the standards being addressed here are also being addressed by children of #2571965: [meta] Fix coding standards in core - once those are addressed, we can review this patch to see if there's anything left to do.

Note that the last patch brought back obsolete template-related classes that were removed in 8.1.x.

heddn’s picture

Issue tags: -Novice

While at some point this might become novice, I'm going to remove that until we are more certain on what should be accomplished in this issue.

alexpott’s picture

Status: Postponed » Closed (duplicate)

Thank you for your work on cleaning up Drupal core's code style!

In order to fix core coding standards in a maintainable way, all our coding standards issues should be done on a per-rule basis across all of core, rather than fixing standards in individual modules or files. We should also separate fixes where we need to write new documentation from fixes where we need to correct existing standards. This all should be done as part of #2571965: [meta] Fix coding standards in core. A good place to start is the child issues of #2572645: [plan] Fix 'Drupal.Commenting.FunctionComment' coding standard.

For background information on why we usually will not commit coding standards fixes that aren't scoped in that way, see the core issue scope guidelines, especially the note about coding standards cleanups. That document also includes numerous suggestions for scoping issues including documentation coding standards cleanups.

Contributing to the overall plan above will help ensure that your fixes for core's coding standards remain in core the long term.

I'm going to close this issue - migrate is part of core and should not be dealt with separately.