Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add support for loading nodes, fields, and paragraphs. #34

Merged
merged 18 commits into from
Jun 27, 2018

Conversation

damontgomery
Copy link
Contributor

@damontgomery damontgomery commented May 29, 2018

Nodes can be loaded by type / title / language.
Fields can be loaded.
Entity references are loaded.
Special handling is used for fields with "nothing" values, they are checked to see if they are empty.

Special handling for:

  • Entity references which includes nodes, terms, media
  • Long text
  • Date (not yet ranges)
  • Links
  • Translations
  • Paragraph types in fields
  • Fields on paragraphs

Example:

Scenario: Blog post exists
  When I examine the "blog_post" node with title "My post"
  Then entity field "field_simple_example" should contain "test"
  And entity field "field_is_a_thing" should contain "nothing"
  # Example entity reference
  And entity field "field_authors" should contain "John Smith"
  And paragraph field "field_paragraphs" should be of type "my_paragraph_type"

Scenario: Paragraph fields
  When I examine the "page" node with title "My page"
  And I examine paragraph "5" on the "field_paragraphs" field
  Then entity field "field_content" should contain "Example"

Scenario: Paragraph fields in English
  When I examine the "page" node with title "My page" in "en"
  And I examine paragraph "5" on the "field_paragraphs" field
  Then entity field "field_content" should contain "Example"

Nodes can be loaded by type / title.
Fields can be loaded.
Entity references are loaded.
Special handling is used for fields with "nothing" values, they are checked to see if they are empty.

Example:
```
Scenario: Blog post exists
  Given I examine the "blog_post" node with title "My post"
  Then entity property "title" should be "My post"
  And entity field "field_simple_example" should contain "test"
  And entity field "field_is_a_thing" should contain "nothing"
  # Example entity reference
  And entity field "field_authors" should contain "John Smith"
```
@damontgomery damontgomery requested a review from becw May 29, 2018 18:14
@damontgomery
Copy link
Contributor Author

Going to work on this a little more.

Since properties and fields are accessed the same way.
After testing on more examples.
Fix docs around input parameters
Add docs for $currentEntity
The default seems to be `name` rather than `title` which is used for nodes.
Paragraphs don't have titles, but we can check the type. A new pattern has been added.
Paragraphs are entity references, so we throw an error when the general field pattern is being used since we aren't checking for value / title.
@damontgomery damontgomery reopened this May 30, 2018
@damontgomery
Copy link
Contributor Author

I missed out on the shared node extension. :(

This was needed before, but I forgot to export it.
@damontgomery damontgomery reopened this May 30, 2018
Can be used like this
```
When I examine the "page" node with title "My page"
And I examine paragraph "5" on the "field_paragraphs" field
Then entity field "field_content" should contain "Example"
```
@damontgomery damontgomery changed the title Add support for loading nodes and fields. Add support for loading nodes, fields, and paragraphs. Jun 5, 2018
@damontgomery
Copy link
Contributor Author

Added support for loading paragraphs.

@damontgomery
Copy link
Contributor Author

I've included the link work in #35 and also added support for translations.

Copy link
Member

@becw becw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done a visual review of the code here -- is there any additional way you'd like me to test this?

BTW, the paragraph handling you added is pretty cool!

@@ -340,43 +399,66 @@ public function assertNotEntityFieldValue($field, $value)

}//end assertNotEntityFieldValue()

/**
* Verify that a field contains a value.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this line of the comment is not accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'll update this.


switch ($entity->getEntityTypeId()) {
case 'node':
$title = $entity->title->value;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use $entity->label() here instead of relying on local logic for finding entity labels?

Also, I don't think Paragraph entities will come up here, since they are referenced by the "Entity Reference Revisions" field type, rather than the "Entity Reference" field type.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

... ah, except for the assertEntityFieldValueEntityReferenceRevisions methods below! I see.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I should be able to use $entity->label() for everything but paragraphs (their label isn't useful). I can update this and it should be a little clearer.

return node_load($nid);
} else if (empty($entities['node']) === false && count($entities['node']) > 1) {
return $node;
} else if (count($entities) > 1) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this condition won't ever be hit, even when it should be -- the logic above seems to load and return the first nid in the array, regardless of whether the array has more than one result. Is this accurate?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 51 has if (count($entities) === 1) {, so the node will only be loaded if there is a single match. I've definitely hit this many times during tested and used a different node for testing. The logic is from the D7 version and I think it makes sense since there is no good way to know which node to use.

I think some of the confusion is that $nid = array_shift($entities); is used to get the first element. I think the keys are node IDs, so $entities[0] may not work but array_keys($entities)[0] would if that is clearer.

@becw becw assigned damontgomery and unassigned becw Jun 27, 2018
- Update paragraph assert method description.
- Replace switch statement with generalized `$entity->label()`.
- Add comment around node loading logic.
@damontgomery damontgomery force-pushed the feature/entity-context branch from 93e4791 to 9d3bbe5 Compare June 27, 2018 14:23
@damontgomery
Copy link
Contributor Author

@becw,

Thanks for the feedback. I've made the updates you requested. This has been working for us on our project. Please let me know if you want to see some of it in detail on our project.

I'm good with the code as a first version.

If it looks good, do you want to merge it?

@becw becw merged commit 9708647 into palantirnet:drupal8 Jun 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants