Problem/Motivation

The API documentation of MigrateSourceInterface::getIds() needs minor improvements.

Many improvements discussed under this issue were already done as part of #2783715: Entity destination ID schema should match entity ID definition. Therefore the scope of this issue was reduced to minor improvements of the phrasing.

Proposed resolution

Improve the phrasing.

Remaining tasks

Review
Commit

User interface changes

None.

API changes

None.

Data model changes

None.

Original report by generalredneck

currently the documentation reads

Return value

array Array keyed by source field name, with values being a schema array describing the field (such as ['type' => 'string]).

It would be helpful if this was more thoroughly documented. Example, there is the ['alias'] key used in Drupal\node\Plugin\migrate\source\d7\Node. Also there is lack in the documentation of the allowed types... Integer and String are the only ones I know of so far.

Members fund testing for the Drupal project. Drupal Association Learn more

Comments

generalredneck created an issue. See original summary.

mikeryan’s picture

The schema arrays here are the same ones used in hook_schema() (see https://www.drupal.org/node/159605) - perhaps an @see would be helpful.

The 'alias' is a migrate extension, that should be documented here.

eojthebrave’s picture

Regarding the 'alias' key. It looks like this is currently only used in \Drupal\migrate\Plugin\migrate\source\SqlBase::initializeIterator, where it's used to determine which field from the source query is the one in question.

Example:

$field_name = $field_schema['alias'] . '.' . $this->query->escapeField($field_name);

So maybe adding something like; "In addition, the migrate system uses an 'alias' key to distinguish between ambiguous field names in some situations. For example when a SQL source query joins two tables with the same field. Providing an optional alias results in using {alias}.{field_name} being used as the ID field."

Or something like that. While I understand what is going on here I had a hard time figuring out how to explain this in a generic way.

I find it a little bit weird that this is documented on the interface, but not necessarily implemented every time. For example, how does the 'alias' key apply if you're using a JSON source? How would you even know what to set it to in that case?

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

quietone’s picture

Make this a child of the migration documentation Meta issue.

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.

quietone’s picture

Status: Active » Needs review
Issue tags: -Needs Documentation
FileSize
1.61 KB

The documentation for getIds was greatly improved by @claudiu.cristea in #2783715: Entity destination ID schema should match entity ID definition. I've added the suggested text in #3. Also moved the first example to it's own paragraph and added an 'Examples:' heading. At least for me, it is easier to both skim the examples and to find the related text.

phenaproxima’s picture

Assigned: Unassigned » phenaproxima

Self-assigning for review.

phenaproxima’s picture

Assigned: phenaproxima » Unassigned
Status: Needs review » Needs work
  1. +++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
    @@ -82,7 +85,10 @@ public function __toString();
    +   *   distinguish between ambiguous field names, for example, when a SQL source
    

    Let's say "column names", since "field" is a loaded term in Drupal. Also, can we have a hyphen after "field names", not a comma?

  2. +++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
    @@ -82,7 +85,10 @@ public function __toString();
    +   *   query joins two tables with the same field. Providing an optional alias
    

    "Same field" is weird phrasing. How about "...joins two tables which have some of the same column names"? Let us also strike the word "optional" from this line; alternately, we can remove the final sentence altogether because it is an implementation detail specific to SqlBase and therefore arguably does not belong on the interface.

gaurav.kapoor’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
945 bytes
phenaproxima’s picture

Status: Needs review » Needs work

Thanks! Small thing:

+++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
@@ -86,9 +86,8 @@
+   *   distinguish between ambiguous column names- for example, when a SQL source

There needs be a space after "names", before the hyphen, and "source" exceeds the 80-character limit for the line.

Other than that, setting back to NW until #9.2 is addressed.

gaurav.kapoor’s picture

@phenaproxima Fixed 10.1 and regarding 9.2 as per your suggestion i removed last sentence of the paragraph.Keeping it to Needs work for now.

heddn’s picture

Status: Needs work » Reviewed & tested by the community

Unless I'm missing something here, I think all feedback has been addressed. I did a review of the docs changes and the description of getIds() seems to make sense to me. The two odd things that previously were a bit mysterious are now explained: alias and field storage settings.

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
@@ -82,7 +85,9 @@ public function __toString();
+   *   along the ID definition, is 'alias' used by SqlBase source plugin to

by *the* SqlBase source plugin?

mikeryan’s picture

Actually, should the SqlBase-specific 'alias' really be documented in MigrateSourceInterface?

heddn’s picture

re: #15, I think the idea was to provide an example of how custom values could be stored in the ids. Should this be spelled out more clearly?

pk188’s picture

Status: Needs work » Needs review
FileSize
1.53 KB
heddn’s picture

Status: Needs review » Needs work

Please provide an interdiff. I cannot tell what the changes are without one. See https://www.drupal.org/documentation/git/interdiff

pk188’s picture

Status: Needs work » Needs review
FileSize
766 bytes
heddn’s picture

Status: Needs review » Needs work
+++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
@@ -82,7 +85,9 @@ public function __toString();
    *   definition, can be passed in definitions. The most common setting, passed
...
+   *   along the ID definition, is 'alias' used by the SqlBase source plugin to

I think we can do better English here. The phrasing doesn't flow.

And per Mike's comment, let's add an @see to the SqlBase plugin, and shorten the description here a little more to give the idea that there is use of passing along additional key/values. But to *really* read up on it, go see the examples in their real location.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

masipila’s picture

Assigned: Unassigned » masipila

I'll wrap this quite old api documentation issue later today.

masipila’s picture

Assigned: masipila » Unassigned
Status: Needs work » Needs review
Issue tags: +API Documentation
FileSize
2.11 KB
1.89 KB

One more old API documentation issue to be finalized, here we go.

Re: #20

1. This patch addresses the 'flow' of the phrasing. I also improved the flow of the description of the first code example.

2. There was already @see \Drupal\migrate\Plugin\migrate\source\SqlBase

3. @heddn, did you mean by your last comment in #20 that we should remove the last code example? I did not remove it, I think it is a good example. If the developers are reading the API documentation of the SqlBase source plugin class they can then follow a link to MigrateSourceInterface::getIds() from that context.

4. I also separated the code examples from each other by one newline. The coding standards say that we must not use a blank line between the text that explains the code sample and the code sample itself. The newlines I added are to separate examples from each other i.e. we are not talking about newlines between the code and its description.

Cheers,
Markus

phenaproxima’s picture

Status: Needs review » Needs work

Two supernits! I've outdone myself.

  1. +++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
    @@ -80,9 +84,13 @@ public function __toString();
    +   *   passed along the ID definition is 'alias' used by the SqlBase source
    

    There should be a comma after 'alias'.

  2. +++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
    @@ -80,9 +84,13 @@ public function __toString();
    +   *   example when a SQL source query joins two tables with the same column
    

    There should also be a comma after 'example'.

heddn’s picture

re #23 you've addressed my feedback quite well. There's one more nit I see not called out by Adam. Its pasted below. I think we need the word to connect the phrases more cleanly.

+++ b/core/modules/migrate/src/Plugin/MigrateSourceInterface.php
@@ -80,9 +84,13 @@ public function __toString();
+   *   definition can be added as shown below. The most common setting
+   *   passed along the ID definition is 'alias' used by the SqlBase source

The most common setting passed along to the ID definition...

masipila’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
2.12 KB
931 bytes

Three nits from #24-25 addressed with two line changes, haha, beat you by one! :D

Issue summary updated to help committers.

Cheers,
Markus

heddn’s picture

Status: Needs review » Reviewed & tested by the community

All feedback addressed.

  • Gábor Hojtsy committed 2a9dc18 on 8.5.x
    Issue #2728869 by masipila, gaurav.kapoor, pk188, quietone, heddn,...

  • Gábor Hojtsy committed 444d6c9 on 8.4.x
    Issue #2728869 by masipila, gaurav.kapoor, pk188, quietone, heddn,...
Gábor Hojtsy’s picture

Version: 8.5.x-dev » 8.4.x-dev
Status: Reviewed & tested by the community » Fixed

Thanks all, this is clearly an improvement even if just a small one :)

generalredneck’s picture

I do appreciate the inclusion of the issue creators in the credit... this doesn't happen nearly as often as it should.

Status: Fixed » Closed (fixed)

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