Move logic from the get plugin into the Row class

Discussed briefly in the migrate maintainers meeting w/ @mikelutz @maxocub @heddn and discussed removing the row interface. Also discussed the possibility of later, down the road removing entirely the Get plugin. For now leaving it.

Big +1 for this patch. I find nothing actually wrong with it; just minor bits and bobs.

1. +++ b/core/modules/migrate/src/Row.php
   @@ -109,6 +109,20 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
   +   * @return bool|mixed|null
   +   *   The source or destination property requested.
   

   This can just be mixed|null, and the description should say that NULL is returned if the property doesn't exist.

2. +++ b/core/modules/migrate/src/Row.php
   @@ -109,6 +109,20 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
   +    return reset($properties);
   

   reset() returns FALSE if the array is empty. So we should probably translate that to NULL, like so: return $properties ? reset($properties) : NULL.

3. +++ b/core/modules/migrate/src/Row.php
   @@ -246,6 +260,61 @@ public function setEmptyDestinationProperty($property) {
   +    if (is_array($properties)) {
   +      return $this->getMultiple($properties);
   +    }
   +    $values = $this->getMultiple([$properties]);
   

   If you cast a string to array, you get [$string]. So this can just be $values = $this->getMultiple((array) $properties).

Only a few small things, and then this is ready to go. I really love the test coverage -- seems quite thorough!

1. +++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
   @@ -123,13 +116,31 @@ class MigrationLookup extends ProcessPluginBase implements ContainerFactoryPlugi
   +   * @param \Drupal\migrate\Plugin\MigratePluginManagerInterface|null $process_plugin_manager
   +   *   (deprecated) The $process_plugin_manager parameter is deprecated since
   +   *   version 8.7.x and will be removed in 9.0.0.
   

   Constructors (and plugin implementations in general) are not considered part of the API, so I think we can actually remove this parameter entirely.

2. +++ b/core/modules/migrate/src/Row.php
   @@ -109,6 +109,25 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
   +   * Retrieve a source or destination property.
   +   * If the property key begins with '@' return a destination property,
   +   * otherwise return a source property. the '@' symbol itself can be escaped
   +   * as '@@'. Returns NULL if property is not found.
   

   We can remove the final sentence here, as it is repeated in the @return documentation. Also -- super nitpick time! -- can these lines be nicely formatted and wrapped to 80 characters?

3. +++ b/core/modules/migrate/src/Row.php
   @@ -109,6 +109,25 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
   +    $properties = $this->getMultiple([$property]);
   +    return reset($properties);
   

   Nitpick, but is there any reason not to have __get() essentially be an alias of get()? So the whole body of __get() could be return $this->get($property).

4. +++ b/core/modules/migrate/src/Row.php
   @@ -246,6 +265,61 @@ public function setEmptyDestinationProperty($property) {
   +   * @param array|string $properties
   

   Should be @param string|string[], since AFAIK this cannot be an array of anything but strings.

5. +++ b/core/modules/migrate/src/Row.php
   @@ -246,6 +265,61 @@ public function setEmptyDestinationProperty($property) {
   +   * @param array $properties
   

   Can this be string[]?

6. +++ b/core/modules/migrate/src/Row.php
   @@ -246,6 +265,61 @@ public function setEmptyDestinationProperty($property) {
   +      if ($property[0] == '@') {
   

   I'd say we should use ===, but I'm not sure if that, unlikely as it is, will break existing code somewhere. I guess we'll leave it for now.

7. +++ b/core/modules/migrate/tests/src/Unit/RowTest.php
   @@ -271,4 +308,148 @@ public function testMultipleDestination() {
   +   * @param bool $is_stub
   +   *   Whether this row is a stub row.
   

   This should mention that $is_stub defaults to FALSE.

+++ b/core/modules/migrate/src/Row.php
@@ -112,10 +112,9 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
+   * '@' return a destination property, otherwise return a source property. the

The tiniest thing -- "the" needs to be capitalized after the period.

No need to go back to "needs review"; this is RTBC from me once green on all backends. Awesome!

1. +++ b/core/modules/migrate/src/Row.php
   @@ -109,6 +109,23 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
   +  public function __get($property) {
   

   Why do do we need this magic?

2. +++ b/core/modules/migrate/src/Row.php
   @@ -246,6 +263,61 @@ public function setEmptyDestinationProperty($property) {
   +   * @param string|string[] $properties
   +   *   The property to get, or an array of properties to get.
   ...
   +  public function get($properties) {
   ...
   +   * @param string[] $properties
   ...
   +  public function getMultiple(array $properties) {
   

   Why do we need a get that supports an array when we have getMultiple()?

Feedback addressed. Back to RTBC.

One thought I have is after this patch what is the point in getSourceProperty() / getDestinationProperty()? It would be handy if the document described when you should use the new methods over the existing ones.

Also the place where the methods have been added to the Row class seems inconsistent. I would have thought the two new methods would come before or after any getSource* or getDestination*

+++ b/core/modules/migrate/tests/src/Unit/process/MigrationLookupTest.php
@@ -13,6 +14,7 @@
+use Drupal\Tests\Traits\ExpectDeprecationTrait;

Not used.

So I guess that leads me to wondering how useful will this really be for contrib and if the flip case is true - in that it might cause some contrib / custom plugins to use the ->get() method when they should be using one of the others?

At least the new document makes it clearer about when to use the different things.

re #27: after reviewing the patch, the CR and the docs on the new methods, it does make sense to add this to Row. Because we use the 'get' logic all over the place in custom and contrib process plugins. And to make it generic, we need a way to get those values from either source or destination. We already had that code duplication, just it was stuffed down in Get process plugin. And in retrospect, we probably don't need the getSource or getDestination methods as public. We might even want to eventually deprecate them or see if we can move them to protected. Where the data is stored is an implementation details irrelevant to callers.

Plenty of test coverage. CR is good. And since this is to help us improve migration_lookup, let's get this in.

+1 This is really nice work and the two CRs are spot on.

I have one personal reservation and that is the potential loss of getSourceProperty and getDestinationProperty which, I think, are easier to understand than get('name') or get('@name'). Just have to wait and see on that one.

Committed a9f1973 and pushed to 8.7.x. Thanks!

diff --git a/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
index 0643b93f65..aa7483a95a 100644
--- a/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
+++ b/core/modules/migrate/src/Plugin/migrate/process/MigrationLookup.php
@@ -4,7 +4,6 @@
 
 use Drupal\Core\Plugin\ContainerFactoryPluginInterface;
 use Drupal\migrate\MigrateSkipProcessException;
-use Drupal\migrate\Plugin\MigratePluginManagerInterface;
 use Drupal\migrate\Plugin\MigrationPluginManagerInterface;
 use Drupal\migrate\Plugin\MigrateIdMapInterface;
 use Drupal\migrate\ProcessPluginBase;

Fixed unused use on commit.