Skip to content

Reduced single-property fields to scalars.#385

Merged
fubhy merged 5 commits intodrupal-graphql:8.x-3.xfrom
blazeyo:377-reduce-single-property-fields-to-scalars
Oct 16, 2017
Merged

Reduced single-property fields to scalars.#385
fubhy merged 5 commits intodrupal-graphql:8.x-3.xfrom
blazeyo:377-reduce-single-property-fields-to-scalars

Conversation

@blazeyo
Copy link
Copy Markdown
Contributor

@blazeyo blazeyo commented Oct 15, 2017

Issue: #377

$type = $definition['type'];
$result = $item->$property;

if ($type == 'Int') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can make this a strict comparison.

if ($type == 'Int') {
$result = (int) $result;
}
elseif ($type == 'Float') {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Same here. ===

$properties = $item->getProperties(TRUE);
if (count($properties) == 1) {
yield $this->resolveItem($item);
}else {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We write else on a new line in Drupal.

foreach ($value->get($fieldName) as $item) {
yield $item;
$properties = $item->getProperties(TRUE);
if (count($properties) == 1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be a strict comparison


$derivative['type'] = $this->typeMapper->typedDataToGraphQLFieldType($property);
$derivative['property'] = reset($keys);
}else {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We write else on a new line.

$derivative['type'] = EntityFieldType::getId($entityTypeId, $fieldName);
}

if ($bundleId) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Always check empty / !empty instead of just checking the truthy value of a parameter.

];

$properties = $definition->getPropertyDefinitions();
if (count($properties) == 1) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This should be a strict comparison ===.

@blazeyo
Copy link
Copy Markdown
Contributor Author

blazeyo commented Oct 16, 2017

All the comments addressed.

$definition = $this->getPluginDefinition();
$property = $definition['property'];
$type = $definition['type'];
$result = $item->$property;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'd prefer an explicit $item->get($property) here.

$derivative['type'] = EntityFieldType::getId($entityTypeId, $fieldName);
}

if (is_null($bundleId)) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This whole condition block could be written in-line.

* @param null|string $bundleId
* Bundle id.
*/
protected abstract function getDerivativesFromPropertyDefinitions($entityTypeId, FieldStorageDefinitionInterface $definition, array $basePluginDefinition, $bundleId = NULL);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

abstract protected function, not protected abstract function.

@pmelab
Copy link
Copy Markdown
Contributor

pmelab commented Oct 16, 2017

I'm sorry to step in so late. We had the idea to attach features of current display-driven fields to raw value types (e.g. the derivative field to the raw image field type). This won't work for scalars.

Are we able to check if there is a plugin that binds to this type first before simplifying it?

@fubhy fubhy merged commit 0a24da8 into drupal-graphql:8.x-3.x Oct 16, 2017
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.

3 participants