Fixes to docs and code style for migrate/src/*.php files

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

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

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

Yeah, that works for me.

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.

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

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.

@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.

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 PHP 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.)

Continued with the work but not finished it.

Switching to Needs Review to test the above patch

@xjm It looks like good to me.

Please have a review.

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.

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.

Thanks hussainweb. Will review against 8.1.x.

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

Will add an issue summary to this ticket.

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

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

Updating the Issue Summary

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

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

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.

Previous comment documents merge conflict resolutions

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!

#19 - rerolled

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

At least some of the standards being addressed here are also being addressed by children of #2571965: [meta] Fix PHP 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.

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.

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 PHP coding standards in core. A good place to start is the child issues of #2572645: [Meta] 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.