Comments

quietone created an issue. See original summary.

quietone’s picture

Status: Active » Needs review
StatusFileSize
new2.52 KB
phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

ultimike’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/process/Get.php
@@ -7,9 +7,74 @@
+ * get also supports a list of source properties. If this list contains an empty
+ * element then the current value will be used. This makes it impossible to
+ * reach a source property with an empty string as its name.

I think an example of this would be useful. The explanation makes sense, but an example would be very helpful. I did a quick search in core and didn't find anywhere multiple source properties are used.

Hmm - well, when using the static map process plugin, using multiple source properties is a somewhat common thing...

jofitz’s picture

Assigned: phenaproxima » Unassigned
StatusFileSize
new1.45 KB
new2.78 KB

Added an example of multiple sources, as suggested by @ultimike.

phenaproxima’s picture

+++ b/core/modules/migrate/src/Plugin/migrate/process/Get.php
@@ -36,9 +36,25 @@
+ * If the list of source properties contains an empty element then the current
+ * value will be used. This makes it impossible to reach a source property with
+ * an empty string as its name.

I think we should expand on this a bit. Maybe an example? If this is just something defensive that the plugin does internally, maybe we should simply omit this passage.

heddn’s picture

Status: Needs review » Needs work

For a non-English speaker, the docs could be clearer. What is it doing? It is getting a value from the Row. Let's describe it then put in an example.

quietone’s picture

One small thing.

+++ b/core/modules/migrate/src/Plugin/migrate/process/Get.php
@@ -7,9 +7,90 @@
+ * Available configuration keys

s/keys/keys:/

yogeshmpawar’s picture

StatusFileSize
new2.78 KB
new538 bytes

Changes done as per comment #9 & also added interdiff.

yogeshmpawar’s picture

Status: Needs work » Needs review
phenaproxima’s picture

Status: Needs review » Needs work

Kicking back to NW until #8 is addressed.

jofitz’s picture

Status: Needs work » Needs review
StatusFileSize
new2.56 KB
new1.43 KB

Tried to improve the readability of the docs in response to #8. Also corrected an indentation error.

phenaproxima’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate/src/Plugin/migrate/process/Get.php
@@ -7,9 +7,87 @@
+ * Available configuration keys:
+ *   - source: Source property.

I think the properties have to be flush with the word "Available".

Other than that, this looks pretty good. RTBC from me once this nit is fixed.

jofitz’s picture

Status: Needs work » Needs review
StatusFileSize
new2.56 KB
new530 bytes

Those indentations catch me out so often! Fixed now.

phenaproxima’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 4ea838b to 8.4.x and 6e953b8 to 8.3.x. Thanks!

  • alexpott committed 4ea838b on 8.4.x
    Issue #2845482 by Jo Fitzgerald, Yogesh Pawar, quietone, phenaproxima,...

  • alexpott committed 6e953b8 on 8.3.x
    Issue #2845482 by Jo Fitzgerald, Yogesh Pawar, quietone, phenaproxima,...

Status: Fixed » Closed (fixed)

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