Remove unused $keep variable from migrate sources

IMHO no, there is already lots of TODO's, so it's fine with one patch.
BTW, help on reviewing patches about its documentation and code style is really welcome ;-)

I can pick this up. Do you want me to create a branch for the documentation fixes and such? I have PhpStorm and it has found many easy fixes to apply.

@cosmicdreams, if you have git access a topic branch is fine with me. If not one patch is ok as mentioned before.

1. +++ b/lib/Drupal/migrate/DrupalMessage.php
   @@ -7,8 +7,17 @@
   +/**
   + * Class DrupalMessage
   + *
   + * @package Drupal\migrate
   + */
   
   +++ b/lib/Drupal/migrate/MigrateExecutable.php
   @@ -59,8 +60,18 @@ class MigrateExecutable {
   +  /**
   +   * @var int
   +   */
   ...
   +  /**
   +   * @param MigrationInterface $migration
   +   * @param MigrateMessageInterface $message
   +   *
   +   * @throws \Exception
   +   */
   

   stuff like this looks automatically generated and not then gone over by a human yet (https://drupal.org/node/1354)

   a mix of both (human and automatic) is probably what we want.

2. +++ b/lib/Drupal/migrate/Row.php
   @@ -116,6 +116,7 @@ class Row {
   +    return NULL;
   
   +++ b/lib/Drupal/migrate/Source.php
   @@ -8,6 +8,8 @@
   +use Drupal\migrate\Plugin\MigrateIdMapInterface;
   +use MigrateMap;
   

   so.. code changes too? hm. then I think it should be in the main issue, no?

See also #2125717-25: Migrate in core: patch #1

[edit!:]
Oh. Having read some stuff in the sandbox project page and the g.d.o info, and paid more attention, I see this is an issue in the imp module/sandbox issue queue, and the other is the core issue. This is making more sense now.

I will look into the skipping part of Source::prepareRow when I get home; don't try to fix that one; it's not being used ATM. use MigrateMap is clearly autogenerated and broken as that class is from D7.

Otherwise, I added both cosmicdreams and YesCT to maintainers but please exercise caution before committing automatically generated fixes. There are some D7 parts lurking in there.

So I have remove removed the d7/Fields.php which was just confusing -- we are not yet up to D7, it was nowhere near up to date as it was untouched since Oct 4. I have modernized the Source::prepareRow skip part.

pushed just the changes to DrupalMessage http://drupalcode.org/sandbox/chx/2105305.git/commit/13f7703

Let's define exercise caution... oh regarding _automatically_ generated fixes... well in general, to be careful:
I can push changes I *think* will not effect things, and ask questions here for other stuff I'm not sure about. Also, I can run a few tests locally before I push as a sanity check.
And especially for autogenerated things, we can look at them with our eyes and brains. :)

In order to be efficient with running the tests, please could I get some advice on how people are doing that. For example, please share with me the command you do on the command line to run tests for imp. I admit I usually just run tests in my local site using the testing ui page. I've been meaning to get more practiced at better ways and I guess this is a good time.

Thanks a ton.

I just change into the core directory and run phpunit --group migrate. This runs everything but the integration tests. And yes, I have meant that while I am also exercising phpstorm driven development I am just looking at the errors highlighted and fix manually rather than letting phpstorm fix it especially when a large amount of errors come from unported code.

As for #9:

+ * @package Drupal\migrate

I do not think we use @package?

+ *
+ * @return

Empty return? Don't we skip these?

+ * @return mixed

getRowByDestination, I think, should actually return array|FALSE

+ * @todo Review the cases where we need the Migration parameter, can we avoid that?

Maybe. fields() might not need it, indeed. I will think more on this.

+ return NULL;

Um what? return NULL and doing nothing is the exact same thing.

lib/Drupal/migrate/Source.php has been fixed properly.

@inheritdoc is {@inheritdoc}, no? I cound 3711 {@inheritdoc} in core versus 19 @inheritdoc.

* @return \PHPUnit_Framework_MockObject_MockObject|\Drupal\migrate\Entity\MigrationInterface

Fantastic, that's two phpstorms warring :) It's \Drupal\migrate\Entity\MigrationInterface always.

Thanks!

#9 is in.

in Migration.php

  /**
   * The configuration describing the process plugins.
   *
   * @var array
   */
  public $process;

  /**
   * The object which stores all active process plugins.
   *
   * @var \Drupal\Migrate\Plugin\MigrateProcessBag
   */
  protected $migrateProcessBag;

I'm not sure what $process is. It is only used afaik, as an array of configuration for each process plugin and is used to initialize the migrationProcessBag when creating a new migrationProcessBag when called in getProcess on a Migration.

Would it *only* be used for that initialization? If so, maybe we can say that.

why is plugin in quotes? (in Migration.php)
is it not a true plugin?

  /**
   * The source configuration, with at least a 'plugin' key.
   *
   * @var array
   */
  public $source;

I was thinking of putting the nice text

   * Identifiers enable us to track changes and more. Once the destination
   * plugin saves the row and the destination identifier values are known, we
   * save the the values of the source identifiers, the values of the
   * destination identifiers and the row hash in an identifier (id) map. The
   * source identifier and the hash in the map allows for tracking changes for
   * continuous migrations. The source identifier and the destination identifier
   * allows for looking up values based on a previous migration -- if a source
   * had user uid 44 which became user uid 56 on the destination, a source node
   * authored by uid 44 needs to be able to look up that 56.

from
https://drupal.org/node/2127611
in the description of $idmap in Migration.php,
but then I realized that that was another example similar to #17, where idmap is used to initalize $idMapPlugin, a MigrateIdMapInterface, and that the description of what an idmap is in MigrateIdMapInterface is pretty good already.

and since idmap and idMapPlugin seem to be the same conceptually (to me, at this point in time), I'm not sure how to document their differences... accurately, usefully and without repeating what a idmap is.

I get a php error/warning on

class Migration extends ConfigEntityBase implements MigrationInterface {

that it needs to be abstract, or implement more methods.

I stopped at highwater mark. will continue later.

the interdiff is some of the things I tried as I ran into the questions posted in my recent comments.

Not committing cause I dont like it, and want to get feedback.

Overall, do we think migration.php might be a place people would open first to learn about this stuff?

https://drupal.org/node/2127611 is the starting place IMO. I will make sure the codebase is whole again, I might've broken it yesterday, yes.

more docs stuff:
http://drupalcode.org/sandbox/chx/2105305.git/commit/d3631ee
is #21 seemed fine on second look.
Note more todo there.

http://drupalcode.org/sandbox/chx/2105305.git/commit/0494b0d
MigrateException

*code change*:
http://drupalcode.org/sandbox/chx/2105305.git/commit/8684ea1
MigrateException constructor needed to include the params first of the thing it is extending, then add its own. noticed while looking at the type of the third param.

http://drupalcode.org/sandbox/chx/2105305.git/commit/592c694
reorder properties and methods in MigrateException class

http://drupalcode.org/sandbox/chx/2105305.git/commit/34c0ea3
docs, MigrateExecutable, only part way through properties. need to better understand whole thing to write a good class doc though.

*code change*:
http://drupalcode.org/sandbox/chx/2105305.git/commit/c18e89b
removed the first of two timeOptionExceeded checks in a series in MigrateExecutable. checkStatus might push it over, and checking after anyway.

I don't think I have commit rights (at least when I tried to push it failed). So here's a patch.

This patch adds two missing @return tags.

@cosmicdreams: You have 'Write to VCS' access so either chx granted it after you posted #24 or perhaps you cloned the repo before you were given access, in which case you'd need to update the remote url:
git remote set-url origin cosmicdreams@git.drupal.org:sandbox/chx/2105305.git

uh, these are in reverse cron order, most recent commit in this group is first. oh well.
========
Commit 3caddd0 on master
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/3caddd0?hp=1ccf... oops. taking out the array property typehint in Row since string is also sent. updating docs to reflect that.
November 12, 2013 7:05

Commit 1ccfd6c on master
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/1ccfd6c?hp=24dc...
put back id instead of key, and add full path to some classes in docs.

Commit 24dc811 on master
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/24dc811?hp=3415... docs and unused uses, suggested by alexpott. two spaces indent after @param, @param var name, types, descriptions, some spelling.
November 12, 2013 6:52

Commit 3415af6 on master
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/3415af6?hp=67cf... menthod param type hint to include array in Row class. also docs, class name, third person verb on methods, missing @ on return.

Commit 67cf1a7 on master
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/67cf1a7?hp=d3fc... docs, suggestions from alexpott, third person verb oneline summaries, @param, some type corrections in the doc. typehints to come.

Commit d3fc23d on master
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/d3fc23d?hp=4e51... docs, some changes from alexpott, adding period to sentences, clarifying wording, @param and @return

Commit 4e517cb on master
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/4e517cb?hp=f0db... docs, fix plural/singular for delete delete bulk. note no usages of deleteDestination yet.

Commit f0db477 on master
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/f0db477?hp=7edc... docs, @param and descriptions, a todo format fix, function doc for getQualifiedMapTable

Commit 7edcd05 on master
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/7edcd05?hp=5ca7... docs, super minor, just 2 space indent under @param for MigrateIdMapInterface

patch #1 went in.
continuing with docs stuff. just a start, need to read the whole class to get more meaningful docs.
http://drupalcode.org/sandbox/chx/2105305.git/commitdiff/a3cf4f74390f082...

question:
I dont see a source property in MigrateExecutable
but getSource seems to think there is one.
and the class does not extend or implement anything, so I dont see where it thinks it has $this->source

I will look into the skipping part of Source::prepareRow()

So here is a patch to clean-up it

All issues for the D6-D8 migrations are now going into the core issue queue.

re-roll

Re-rolled. I think this should be RTBC.

agree

Drupal\migrate_drupal\Plugin\migrate\source\d6\Comment::prepareRow still has the $keep param.

Also the issue summary and issue title are extremely vague.

added back that hunk

Committed 758267e and pushed to 8.0.x. Thanks!