Add API documentation to the UrlEncode process plugin.

CommentFileSizeAuthor
#11 2845491-11.patch967 bytesjofitz
#11 interdiff-9-11.txt1.77 KBjofitz
#9 2845491-9.patch1.92 KBjofitz
#9 interdiff-2-9.txt749 bytesjofitz
#2 2845491-1.patch1.72 KBquietone
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

quietone created an issue. See original summary.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Active » Needs review
FileSize
2845491-1.patch1.72 KB

Status: Needs review » Needs work

The last submitted patch, 2: 2845491-1.patch, failed testing.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented

The failure doesn't seem related, so retesting.

quietone’s picture

quietone CreditAttribution: quietone as a volunteer commented
Status: Needs work » Needs review

Passed, so time to NR.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
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

ultimike
Primary language English
Location Florida, USA
CreditAttribution: ultimike at DrupalEasy commented 
+++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
@@ -9,10 +9,20 @@
+ * Example:

If the source URL is "http://example.com/this is a url with spaces.html", this process plugin will return "http://example.com/this%20is%20a%20url%20with%20spaces.html"

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
FileSize
interdiff-2-9.txt749 bytes
2845491-9.patch1.92 KB

Add more explanation to the example, as suggested by @ultimike.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Assigned: phenaproxima » Unassigned
Status: Needs review » Needs work 
  1. +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
@@ -9,10 +9,24 @@
+ * Applies urlencoding to the source value.

    'urlencoding' is not a word. Also, kinda passive-voice. Maybe we could rephrase this as "URL-encodes the input value"? 

  2. +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
@@ -9,10 +9,24 @@
+ * The urlencode plugin is needed when the URI is to be opened by a later
+ * migration stage, and the source URI value is not already encoded.

    I think we could strike this entire sentence. It does not explain anything about the plugin itself, just about how it interacts with a migration...under certain circumstances. 

  3. +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
@@ -9,10 +9,24 @@
+ * This will convert the source url 'http://example.com/a url with spaces.html'

    s/url/URL. 

  4. +++ b/core/modules/migrate/src/Plugin/migrate/process/UrlEncode.php
@@ -21,7 +35,22 @@
-   * {@inheritdoc}
+   * Performs the associated process.
+   *
+   * @param string $value
+   *   The input URL
+   * @param \Drupal\migrate\MigrateExecutableInterface $migrate_executable
+   *   The migration in which this process is being executed.
+   * @param \Drupal\migrate\Row $row
+   *   The row from the source to process.
+   * @param string $destination_property
+   *   The destination property currently worked on. This is only used together
+   *   with the $row above.
+   *
+   * @return string
+   *   The sub string of $value.
+   *
+   * @throws \Drupal\migrate\MigrateException

    This should be {@inheritdoc}.

jofitz’s picture

jofitz CreditAttribution: jofitz at ComputerMinds commented
Status: Needs work » Needs review
FileSize
interdiff-9-11.txt1.77 KB
2845491-11.patch967 bytes

Changes in response to the code review by @phenaproxima in #10.

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
CreditAttribution: phenaproxima at Acquia commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

  • xjm committed 8ff5e72 on 8.4.x 
    Issue #2845491 by Jo Fitzgerald, quietone, phenaproxima, ultimike: Add...
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
Status: Reviewed & tested by the community » Fixed

Short and sweet. Thanks, all! Committed to 8.4.x, and backported to 8.3.x as a documentation improvement.

  • xjm committed cc2ebd5 on 8.3.x 
    Issue #2845491 by Jo Fitzgerald, quietone, phenaproxima, ultimike: Add...

Status: Fixed » Closed (fixed)

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