The plugin is often necessary when migrating things that had a serial and we need a machine name and it's best to construct it from the human name. Iit's documented https://drupal.org/node/2135323 and it's tested.

CommentFileSizeAuthor
#11 2150257_11.patch4.72 KBchx
#11 interdiff.txt721 byteschx
#9 2150257_9.patch4.59 KBchx
#9 interdiff.txt1017 byteschx
#7 interdiff.txt1.42 KBchx
#7 2150257_6.patch4.47 KBchx
#5 2150257_5.patch3.62 KBchx
#5 interdiff.txt3.99 KBchx
#3 migrate--machine-name-process-unit-tests--2150257-3.patch2.66 KBmpgeek
machine_name.patch1.63 KBchx
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes
mpgeek’s picture

mpgeek CreditAttribution: mpgeek commented
Assigned: Unassigned » mpgeek
mpgeek’s picture

mpgeek CreditAttribution: mpgeek commented
Status: Needs work » Needs review
FileSize
migrate--machine-name-process-unit-tests--2150257-3.patch2.66 KB

This patch adds the test but i'm getting PHP Fatal error (calling method on non-object) from inside MachineName::getTransliteration. My test string is not over 128 characters long as indicated in the summary, but it's unclear to me why this would be necessary. Either there really is a bug or i'm missing something here. Setting to needs review, hoping for feedback so I can finish this up.

Status: Needs review » Needs work

The last submitted patch, 3: migrate--machine-name-process-unit-tests--2150257-3.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
interdiff.txt3.99 KB
2150257_5.patch3.62 KB

Thanks for the test, a few fixes were necessary: the plugin needed a test class to set the transliteration and also process plugins work pipeline-like, they get the current value of the pipeline as the first argument and spit out the next value in return. The other parameters are making process plugins powerful by making them stateful; the above behaviour would only allow for stateless. Finally, the machine name plugin has no configuration at all.

Congratulations on finding a bug in the plugin (I left in the dot because I copied from BlockBase. Opsie.)

Status: Needs review » Needs work

The last submitted patch, 5: 2150257_5.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Assigned: mpgeek » chx
Status: Needs work » Needs review
FileSize
2150257_6.patch4.47 KB
interdiff.txt1.42 KB

Oh and that above 128 was meant to mean it needs a few Unicode characters not in ASCII: above 128 codepoints. So I added this.

jibran’s picture

jibran
he/him
Primary language Urdu
Location Sydney, Australia
CreditAttribution: jibran commented

Here are some more points and suggestions.

  1. +++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/MachineName.php
@@ -0,0 +1,49 @@
+   * @var \Drupal\Core\Transliteration\PHPTransliteration
...
+   * @return \Drupal\Core\Transliteration\PHPTransliteration

+++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/MachineNameTest.php
@@ -0,0 +1,89 @@
+class MachineNameTest extends MigrateProcessTestCase {

    desc line missing 

  2. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/MachineNameTest.php
@@ -0,0 +1,89 @@
+ * Contains

    Incomplete 

  3. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/MachineNameTest.php
@@ -0,0 +1,89 @@
+  protected $mapJoinable = FALSE;

    @var block missing. 

  4. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/MachineNameTest.php
@@ -0,0 +1,89 @@
+   * Test machine name transformation of non-alphanumeric characters.

    Tests 

  5. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/MachineNameTest.php
@@ -0,0 +1,89 @@
+    $human_name_ascii = 'foo2, the.bar;2*&the%baz!YEE____HaW ';

    YEE___HaW :D 

  6. +++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/MachineNameTest.php
@@ -0,0 +1,89 @@
+namespace Drupal\migrate\Plugin\migrate\process;
+
+use Drupal\Component\Transliteration\TransliterationInterface;
+
+class TestMachineName extends MachineName {
+  public function setTransliteration(TransliterationInterface $transliteration) {
+    $this->transliteration = $transliteration;
+  }
+}

    All kind of doxgen missing i.e. desc block for code,class and function.

chx’s picture

chx CreditAttribution: chx commented
FileSize
interdiff.txt1017 bytes
2150257_9.patch4.59 KB

Fixed the pieces needing fixes. (Test only classes containing two LoC that you can't even load because they are not PSR-0 don't need doxygen.)

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented 
+++ b/core/modules/migrate/lib/Drupal/migrate/Plugin/migrate/process/MachineName.php
@@ -0,0 +1,49 @@
+ * This plugin sets a destination property based on multiple sources and a map.
...
+class MachineName extends PluginBase implements MigrateProcessInterface {

I don't really think the description matches to the actual code.

chx’s picture

chx CreditAttribution: chx commented
FileSize
interdiff.txt721 bytes
2150257_11.patch4.72 KB

How about this?

dawehner’s picture

dawehner
Primary language German
CreditAttribution: dawehner commented
Status: Needs review » Reviewed & tested by the community

Thank you!

webchick’s picture

webchick
she/they
Primary language English
Location Vancouver 🇨🇦
CreditAttribution: webchick commented
Status: Reviewed & tested by the community » Fixed 
+++ b/core/modules/migrate/tests/Drupal/migrate/Tests/process/MachineNameTest.php
@@ -0,0 +1,93 @@
+  /**
+   * @var bool
+   */
+  protected $mapJoinable = FALSE;
+

This variable is a) not documented (neither here nor in the base class — it's @TODO), and b) not used in the code below. I mentioned this to chx, and he said that there is already a follow-up issue to further clean-up mapJoinable at #2152131: Clean up mapJoinable, so this will just get roped into that. Normally this variable is needed, when an actual migration is being tested, but it is not here, so it is superfluous for now, but may not always be, so it's best to keep it here to be safe.

With that follow-up filed, we are probably good to go here for now, so...

Committed and pushed to 8.x. Thanks!

Status: Fixed » Needs work

The last submitted patch, 11: 2150257_11.patch, failed testing.

chx’s picture

chx CreditAttribution: chx commented
Status: Needs work » Fixed

Yeah, we've seen this random fail before...

Status: Fixed » Closed (fixed)

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