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
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

phenaproxima’s picture

Status: Active » Needs review
FileSize
3.16 KB
phenaproxima’s picture

FileSize
4.51 KB

Whoops -- forgot to fix the unit test.

tim.plunkett’s picture

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

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

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.