Problem/Motivation

FieldableEntity provides a base class for migration source plugins from Drupal.

It has two methods, getFields() and getFieldValues(). These explain what they do, but they could do to also explain how they should be used.

See https://api.drupal.org/api/drupal/core%21modules%21node%21src%21Plugin%2... for an example.

Steps to reproduce

Proposed resolution

Remaining tasks

User interface changes

API changes

Data model changes

Release notes snippet

Issue fork drupal-3203009

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim created an issue. See original summary.

Version: 9.2.x-dev » 9.3.x-dev

Drupal 9.2.0-alpha1 will be released the week of May 3, 2021, which means new developments and disruptive changes should now be targeted for the 9.3.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Status: Active » Postponed (maintainer needs more info)

@joachim, I'm not sure what you are looking for here. Can you provide an example of the documentation you'd like to see?

joachim’s picture

Status: Postponed (maintainer needs more info) » Active

Looking at the code, I think I meant that both of them should say they are helpers for prepareRow(), and getFieldValues() should also say that its return value is intended for setSourceProperty().

(I need to try to give more detail in documentation issues, as clearly what seems obvious to me at the time isn't obvious later even to me! sorry!)

quietone’s picture

Status: Active » Needs review
FileSize
1.34 KB

Thanks! How is this?

Having come across quite a few of your documentation issues I agree with your self assessment. Having a sentence or even a phrase to start with would be really helpful. I just assume that you are focused on debugging something and don't want to lose your train of thought.

joachim’s picture

Status: Needs review » Reviewed & tested by the community

> I just assume that you are focused on debugging something and don't want to lose your train of thought.

Yes, it's pretty much that!

Your text looks perfect! Thanks!

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/migrate_drupal/src/Plugin/migrate/source/d7/FieldableEntity.php
@@ -21,6 +21,12 @@ abstract class FieldableEntity extends DrupalSqlBase {
    *
+   * This is typically used in the prepareRow method of a source plugin to get
+   * a list of all the field instances of the entity. A source plugin can then
+   * loop through the list of fields to do any other preparation before
+   * processing the row. Typically, a source plugin will use getFieldValues()
+   * to get the values of each field.
+   *
    * @param string $entity_type
    *   The entity type ID.
    * @param string|null $bundle
@@ -47,6 +53,9 @@ protected function getFields($entity_type, $bundle = NULL) {

@@ -47,6 +53,9 @@ protected function getFields($entity_type, $bundle = NULL) {
   /**
    * Retrieves field values for a single field of a single entity.
    *
+   * This is typically used in the prepareRow method of a source plugin where
+   * the return values are placed on the row source.
+   *

Nit: I think we can drop the 'This is' from both comments. But otherwise looks good.

Gauravmahlawat made their first commit to this issue’s fork.

Gauravvvv’s picture

Status: Needs work » Needs review
Gauravvvv’s picture

MR updated as per #7.

quietone’s picture

@Gauravmahlawat, remember to follow Drupal coding standards. The changes made in the MR should wrap at 80 columns.

#7. Without the 'This is', then sentence needs to change a bit so that there is a comma after 'Typically'. I made a new patch based of the patch in #5 (I am not keen on working with MRs).

joachim’s picture

Status: Needs review » Reviewed & tested by the community

I don't really see how having to use the name of the method in the docblock for that method is better than saying 'This is'; it's definitely not as succinct... But I think it's fine like this.

catch’s picture

Hmm I just meant 'Typically used in...' as in literally drop the two words.

quietone’s picture

Sorry, forgot to add that it wasn't until I read the suggested change that it didn't seem right. So searched, and found that https://dictionary.cambridge.org/example/english/typically, uses the comma after typically when it begins a sentence.

quietone’s picture

Since this is RTBC, let's run the tests.

alexpott’s picture

Version: 9.3.x-dev » 9.2.x-dev
Status: Reviewed & tested by the community » Fixed

Committed and pushed 76f6b690e1 to 9.3.x and a1ed6ed5d0 to 9.2.x. Thanks!

  • alexpott committed 76f6b69 on 9.3.x
    Issue #3203009 by quietone, Gauravmahlawat, joachim, catch: the methods...

  • alexpott committed a1ed6ed on 9.2.x
    Issue #3203009 by quietone, Gauravmahlawat, joachim, catch: the methods...

Status: Fixed » Closed (fixed)

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