Add quotes around property name in exception message in Row.php

@John.nie, thanks for the patch. Adding the name of $id will be helpful.

+++ b/core/modules/migrate/src/Row.php
@@ -104,7 +104,7 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
+        throw new \InvalidArgumentException(t("@key is defined as a source ID but has no value.", ['@key' => $id]));

The @key needs single quotes, "'$key' is defined as a source ID but has no value". Exceptions messages are not translated so the use of t() can be removed. See Php exceptions.

This patch might fix comment #6.

@ravi.shankar, thanks for the patch.

The unit test still needs to be fixed.

+++ b/core/modules/migrate/src/Row.php
@@ -104,7 +104,7 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
-        throw new \InvalidArgumentException("$id is defined as a source ID but has no value.");
+        throw new \InvalidArgumentException("'@key' is defined as a source ID but has no value.", ['@key' => $id]);

IMO, should be just throw new \InvalidArgumentException("'$id' is defined as a source ID but has no value.");, no need to use spritf()

It's unclear what to do here, besides adding single quotes around $id, so adding a "Needs issue summary update" tag

@jian he, thanks for updating the title. I think you are asking for the exception message that is thrown in Row.php to be translated. However, Exception messages in Drupal are not translated. This is in the coding standards for exceptions, it is the 3rd item in Basic conventions. I don't know if there is an issue for adding translation for exception messages.

I'm changing the title based on my understanding. If I am wrong, please say so.

Needs work for fixing the two failing tests.

Here I have tried to fix failed tests of patch #8.

+++ b/core/modules/migrate/src/Row.php
@@ -104,7 +104,7 @@ public function __construct(array $values = [], array $source_ids = [], $is_stub
-        throw new \InvalidArgumentException("$id is defined as a source ID but has no value.");

The format string for sprintf looks more like a format string for the t() function. Sprintf doesn't use an '@' symbol for conversion specifications. But there is no need for sprintf anyway. This just needs single quotes around the string $id so that it becomes '$id'.

Then the exception message in testInvalidSourceIdKeys needs updating. And I think we should an a test of the exception message in testRowWithInvalidData() as well. That needs $this->expectExceptionMessage("'nid' is defined as a source ID but has no value.");

@ravi.shankar, I am not aware of your experience or your knowledge with Drupal so please ignore if I am repeating things you already know. Before uploading a patch to let the testbot test it is better to test the patch locally in your own dev environment. That way you can test your ideas on just the one file you are working on and make multiple iterations easily. Setting up for testing is worth the time and effort. There is a handbook section that covers all aspects of PHPUnit in Drupal. Cheers.

@quietone Thanks

Added a patch that might fix the failed tests.

  public function __construct(array $values = [], array $source_ids = [], $is_stub = FALSE) {
    $this->source = $values;
    $this->sourceIds = $source_ids;
    $this->isStub = $is_stub;
    foreach (array_keys($source_ids) as $id) {
      if (!$this->hasSourceProperty($id)) {
        throw new \InvalidArgumentException("'$id' is defined as a source ID but has no value.");
      }
    }
  }

The title is "Add property name to exception message in Row.php", but it's impossible to get the source property per !$this->hasSourceProperty($id), so how to add the property name to the exception message?

From my understanding, this issue is just adding single quotes, nothing more.

@jungle, yes it is about adding single quotes and I have finally updated the title.

The patch in #19 adds the missing single quotes around the name of the source id and the test is updated correctly.

Committed 33a6866 and pushed to 9.1.x. Thanks!

As the ID is already included I think this is only a task.