Problem/Motivation

Against best practices, MachineName process plugin uses \Drupal::transliteration(). There doesn't appear to be any reason why it needs to do this.

Proposed Resolution

Inject the transliteration service in the create() method.

Remaining Tasks

SOP...patch, review, commit.

API/UI Changes

None!

CommentFileSizeAuthor
#2 2488126-2.patch4.51 KBphenaproxima
#1 2488126-1.patch3.16 KBphenaproxima

Comments

phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
Status: Active » Needs review
StatusFileSize
new 2488126-1.patch3.16 KB
phenaproxima’s picture

phenaproxima
he/him
Primary language English
Location Massachusetts
commented
StatusFileSize
new 2488126-2.patch4.51 KB

Whoops -- forgot to fix the unit test.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Priority: Minor » Normal

I'm not sure how "inject things properly" fits into the beta evaluation, but this patch is correct. The diff looks like it changes transform() more than it does, here is what git diff shows me after applying the patch:

    * {@inheritdoc}
    */
   public function transform($value, MigrateExecutableInterface $migrate_executable, Row $row, $destination_property) {
-    $new_value = $this->getTransliteration()->transliterate($value, LanguageInterface::LANGCODE_DEFAULT, '_');
+    $new_value = $this->transliteration->transliterate($value, LanguageInterface::LANGCODE_DEFAULT, '_');
     $new_value = strtolower($new_value);
     $new_value = preg_replace('/[^a-z0-9_]+/', '_', $new_value);
     return preg_replace('/_+/', '_', $new_value);
   }

The last submitted patch, 1: 2488126-1.patch, failed testing.

chx’s picture

chx commented
Status: Needs review » Reviewed & tested by the community

> I'm not sure how "inject things properly" fits into the beta evaluation, but this patch is correct.

Migrate is beta exempt. Thanks for the speedy review!

alexpott’s picture

alexpott
he/they
Primary language English
Location 🇪🇺🌍
commented
Status: Reviewed & tested by the community » Fixed

Yep migrate is beta exempt. Committed f52cc18 and pushed to 8.0.x. Thanks!

  • alexpott committed f52cc18 on 8.0.x 
    Issue #2488126 by phenaproxima: MachineName process plugin should use...

Status: Fixed » Closed (fixed)

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