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

EZP-31078: Enhanced Content Type Value Object #2839

Merged
merged 5 commits into from
Dec 20, 2019

Conversation

adamwojs
Copy link
Member

@adamwojs adamwojs commented Oct 25, 2019

Question Answer
JIRA issue EZP-31078
Bug/Improvement no
New feature yes
Target version master
BC breaks yes
Tests pass yes
Doc needed yes

Changelog

  1. Introduced eZ\Publish\API\Repository\Values\ContentType\FieldDefinitionCollection which takeover responsibility of providing access field definitions:
interface FieldDefinitionCollection extends Countable, IteratorAggregate, ArrayAccess
{
    /**
     * This method returns the field definition for the given identifier.
     *
     * @param string $fieldDefinitionIdentifier
     *
     * @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition|null
     */
    public function get(string $fieldDefinitionIdentifier): ?FieldDefinition;

    /**
     * This method returns true if the field definition for the given identifier exists.
     *
     * @param string $fieldDefinitionIdentifier
     *
     * @return bool
     */
    public function has(string $fieldDefinitionIdentifier): bool;

    /**
     * Return first element of collection.
     *
     * @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition|null
     */
    public function first(): ?FieldDefinition;

    /**
     * Return last element of collection.
     *
     * @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition|null
     */
    public function last(): ?FieldDefinition;

    /**
     * Checks whether the collection is empty (contains no elements).
     *
     * @return bool TRUE if the collection is empty, FALSE otherwise.
     */
    public function isEmpty(): bool;

    /**
     * Returns all the elements of this collection that satisfy the predicate p.
     * The order of the elements is preserved.
     *
     * @param Closure $p The predicate used for filtering.
     *
     * @return FieldDefinitionCollection A collection with the results of the filter operation.
     */
    public function filter(Closure $p): FieldDefinitionCollection;

    /**
     * Returns field definitions with given field type identifier.
     *
     * @param string $fieldTypeIdentifier
     *
     * @return FieldDefinitionCollection A collection with the results of the filter operation.
     */
    public function filterByType(string $fieldTypeIdentifier): FieldDefinitionCollection;

    /**
     * Returns field definitions with given group.
     *
     * @param string $fieldGroup
     *
     * @return FieldDefinitionCollection A collection with the results of the filter operation.
     */
    public function filterByGroup(string $fieldGroup): FieldDefinitionCollection;

    /**
     * Applies the given function to each element in the collection and returns
     * a new collection with the elements returned by the function.
     *
     * @param Closure $p The predicate.
     *
     * @return array
     */
    public function map(Closure $p): array;

    /**
     * Tests whether the given predicate p holds for all elements of this collection.
     *
     * @param Closure $p The predicate.
     *
     * @return bool TRUE, if the predicate yields TRUE for all elements, FALSE otherwise.
     */
    public function all(Closure $p): bool;

    /**
     * Tests for the existence of an element that satisfies the given predicate.
     *
     * @param Closure $p The predicate.
     *
     * @return bool TRUE if the predicate is TRUE for at least one element, FALSE otherwise.
     */
    public function any(Closure $p): bool;

    /**
     * Tests for the existence of an field definition with given field type identifier.
     *
     * @param string $fieldTypeIdentifier
     *
     * @return bool TRUE if the predicate is TRUE for at least one field definition, FALSE otherwise.
     */
    public function anyOfType(string $fieldTypeIdentifier): bool;

    /**
     * Tests for the existence of an field definition in given field group.
     *
     * @param string $fieldGroup
     *
     * @return bool TRUE if the predicate is TRUE for at least one field definition, FALSE otherwise.
     */
    public function anyInGroup(string $fieldGroup): bool;

    /**
     * Partitions this collection in two collections according to a predicate.
     *
     * @param Closure $p The predicate on which to partition.
     *
     * @return FieldDefinitionCollection[] An array with two elements. The first element contains the collection
     *                      of elements where the predicate returned TRUE, the second element
     *                      contains the collection of elements where the predicate returned FALSE.
     */
    public function partition(Closure $p): array;

    /**
     * Gets a native PHP array representation of the collection.
     *
     * @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition[]
     */
    public function toArray(): array;
}
  1. Changed return type of getFieldDefinitions() from array to FieldDefinitionCollection.

  2. Added eZ\Publish\API\Repository\Values\ContentType\ContentType methods:

  • hasFieldDefinition($fieldDefinitionIdentifier): bool
  • hasFieldDefinitionOfType(string $fieldTypeIdentifier): bool
  • getFieldDefinitionsOfType(string $fieldTypeIdentifier): FieldDefinitionCollection
  • getFirstFieldDefinitionOfType(string $fieldTypeIdentifier): ?FieldDefinition

Use Cases

getFirstFieldDefinitionOfType

The typical use case for getFirstFieldDefinitionOfType is the search for singular field type (ref. \eZ\Publish\API\Repository\FieldType::isSingular). For example:

// Search for the first ezuser field type in content type
$userFieldDefinition = null;
foreach ($userCreateStruct->contentType->getFieldDefinitions() as $fieldDefinition) {
if ($fieldDefinition->fieldTypeIdentifier == 'ezuser') {
$userFieldDefinition = $fieldDefinition;
break;
}
}

public function getFirstFilledImageFieldIdentifier(Content $content)
{
foreach ($content->getFieldsByLanguage() as $field) {
$fieldTypeIdentifier = $content->getContentType()
->getFieldDefinition($field->fieldDefIdentifier)
->fieldTypeIdentifier;
if ($fieldTypeIdentifier !== 'ezimage') {
continue;
}
if ($this->fieldHelper->isFieldEmpty($content, $field->fieldDefIdentifier)) {
continue;
}
return $field->fieldDefIdentifier;
}
return null;
}

hasFieldDefinition and hasFieldDefintionWithType

hasFieldDefinition and hasFieldDefintionWithType is just syntax sugar which improvs code readability.

if ($contentType->getFieldDefinition('short_description') !== null) {
   # ... 
}

VS

if ($contentType->hasFieldDefinition('short_description')) {
   # ... 
}

TODO:

  • Implement feature / fix a bug.
  • Update /doc/bc/changes-8.0.md
  • Check dependent packages
  • Implement tests.
  • Fix new code according to Coding Standards ($ composer fix-cs).
  • Ask for Code Review.

@adamwojs adamwojs self-assigned this Oct 25, 2019
@adamwojs adamwojs changed the title Enhanced eZ\Publish\API\Repository\Values\ContentType\ContentType VO Enhanced ContentType VO Oct 25, 2019
*
* @return bool
*/
public function hasFieldDefinitionWithType(string $fieldTypeIdentifier): bool
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not returning the number of fields of the given type? The returned number can be used in the same IFs as a boolean would do. It also allows for more complex scenarios (eg. a Content has 2 images => which one to map for our needs ?)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also: given that this method does not optimize away any processing, as it is seems a bit pointless... The user might just put in her code the same check if ($ft->getFieldDefinitionWithType($fieldTypeIdentifier) !== null)

Copy link
Member

Choose a reason for hiding this comment

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

It would sound good with a countFieldDefinitionsWithType().

@bdunogier bdunogier changed the title Enhanced ContentType VO Enhanced ContentType Value Object Oct 25, 2019
@bdunogier bdunogier changed the title Enhanced ContentType Value Object Enhanced Content Type Value Object Oct 25, 2019
*
* @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition
*/
public function getFieldDefinitionWithType(string $fieldTypeIdentifier): ?FieldDefinition
Copy link
Contributor

Choose a reason for hiding this comment

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

'ofType' instead 'withType' ?

Copy link
Contributor

@gggeek gggeek Oct 25, 2019

Choose a reason for hiding this comment

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

also, might add 2nd param int $offset = 0. This way we could get the 1st image field, the 2nd one, etc...

Copy link
Member

Choose a reason for hiding this comment

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

Do we need getFirstFieldDefinitionWithType() and getFieldDefinitionsWithType() ? My question about the use-case for this still stands :)

Copy link

@hknezevic hknezevic Oct 25, 2019

Choose a reason for hiding this comment

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

@bdunogier if you have the getFieldDefinitionsWithType($limit, $offset), I don't think both the getFirstFieldDefinitionWithType() or getFieldDefinitionWithType() method would be needed at all.

@emodric
Copy link
Contributor

emodric commented Oct 25, 2019

Hm. Wouldn't it be more useful to have getFieldDefinitionsWithType and getFieldDefinitionsWithTypeCount? Especially, since oneliners like getFieldDefinitionsWithType('ezimage')[0] ?? null would be possible.

Having the method only return the first definition with a specific field type is most of the time limiting.

@hknezevic
Copy link

getFieldDefinitionWithType() only fetches the first match.
Personally I would prefer to have the count() method returning the number of field definitions for the given type, and another method allowing me to fetch all field definitions of the given type.

Typical example - being able to detect all image or file fields in the content type. Of course this can be done by manually looping through the field definitions, but it would be convenient to have both in PHP and the templates.

@adamwojs adamwojs changed the title Enhanced Content Type Value Object EZP-31078: Enhanced Content Type Value Object Oct 25, 2019
@bdunogier
Copy link
Member

bdunogier commented Oct 25, 2019

About the get...Count(), it loos useful based on the feedback. But then, let's get rid of "get", and call it countFieldDefinitionsWithType.

I kind of like the OfType suggestion. Shorter, and sounds better.

May I also ask what the use-case for finding a field def with a given type is ? If it's generating a thumbnail for a content item, we actually have a feature dedicated to that in store, that does it better and doesn't spread the logic around (similar to content object name).

@gggeek
Copy link
Contributor

gggeek commented Oct 25, 2019

@emodric while in theory getFieldDefinitionsWithType('ezimage')[0] ?? null is short enough to be useable, in practice it makes my head hurt. Must be my age... ;-)

@emodric
Copy link
Contributor

emodric commented Oct 25, 2019

Hah, maybe :) I find it convenient and readable enough to clearly communicate intent. In any case, having a method return only the first match is way less usable.

@adamwojs
Copy link
Member Author

adamwojs commented Oct 26, 2019

Thank you for a feedback. While reading comments about getFieldDefinitionsWithType I get the idea to maybe introduce much more rich API for field definitions (heavily inspired on Doctrine Collections).

namespace eZ\Publish\API\Repository\Values\ContentType;

use Closure;
use Countable;
use IteratorAggregate;

interface FieldDefinitionCollection extends Countable, IteratorAggregate
{
    /**
     * This method returns the field definition for the given identifier.
     *
     * @param string $fieldDefinitionIdentifier
     *
     * @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition|null
     */
    public function get(string $fieldDefinitionIdentifier): ?FieldDefinition;

    /**
     * This method returns true if the field definition for the given identifier exists.
     *
     * @param string $fieldDefinitionIdentifier
     *
     * @return bool
     */
    public function has(string $fieldDefinitionIdentifier): bool;

    /**
     * Return first element of collection
     *
     * @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition|null
     */
    public function first(): ?FieldDefinition;

    /**
     * Return last element of collection
     *
     * @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition|null
     */
    public function last(): ?FieldDefinition;

    /**
     * Checks whether the collection is empty (contains no elements).
     *
     * @return bool TRUE if the collection is empty, FALSE otherwise.
     */
    public function isEmpty(): bool;

    /**
     * Returns all the elements of this collection that satisfy the predicate p.
     * The order of the elements is preserved.
     *
     * @param Closure $p The predicate used for filtering.
     *
     * @return FieldDefinitionCollection A collection with the results of the filter operation.
     */
    public function filter(Closure $p): FieldDefinitionCollection;

    /**
     * Applies the given function to each element in the collection and returns
     * a new collection with the elements returned by the function.
     *
     * @param Closure $p The predicate.
     *
     * @return array
     */
    public function map(Closure $p): array;

    /**
     * Tests whether the given predicate p holds for all elements of this collection.
     *
     * @param Closure $p The predicate.
     *
     * @return bool TRUE, if the predicate yields TRUE for all elements, FALSE otherwise.
     */
    public function all(Closure $p): bool;

    /**
     * Alias for FieldDefinitionCollectionInterface::exists
     *
     * @param Closure $p The predicate.
     *
     * @return bool TRUE if the predicate is TRUE for at least one element, FALSE otherwise.
     */
    public function any(Closure $p): bool;

    /**
     * Tests for the existence of an element that satisfies the given predicate.
     *
     * @param Closure $p The predicate.
     *
     * @return bool TRUE if the predicate is TRUE for at least one element, FALSE otherwise.
     */
    public function exists(Closure $p): bool;

    /**
     * Partitions this collection in two collections according to a predicate.
     *
     * @param Closure $p The predicate on which to partition.
     *
     * @return FieldDefinitionCollection[] An array with two elements. The first element contains the collection
     *                      of elements where the predicate returned TRUE, the second element
     *                      contains the collection of elements where the predicate returned FALSE.
     */
    public function partition(Closure $p): array;

    /**
     * Gets a native PHP array representation of the collection.
     *
     * @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition[]
     */
    public function toArray(): array;
}

FieldDefintionCollection will be returned by \eZ\Publish\API\Repository\Values\ContentType\ContentType::getFieldDefinitions.

\eZ\Publish\API\Repository\Values\ContentType\ContentType::{getFieldDefintion,hasFieldDefinition} method will be still available.

WDYT ?

@gggeek
Copy link
Contributor

gggeek commented Oct 26, 2019

@adamwojs tbh I am not sure that would strike the best balance between flexibility and complexity.
While I all for the flexibility offered by a Collection class in general (see how I used that pervasively in the MigrationBundle), I don't see a lot of usage for it in this specific context.
It seems that most devs who commented so far agree on 2 usecases: get-field-by-identifier and get-list-of-fields-with-given-type. For both those scenarios there is precious little need for first/last/partition/map. Also, the usage of closures for filtering stuff still imposes a somewhat heavy burden on the end dev. Would you still implement getFieldDefinitionsOfType and countFieldDefinitionsOfType in the kernel, or only let the kernel provide the full FD Collection and push onto the users the extraction of the required data?

@adamwojs
Copy link
Member Author

adamwojs commented Oct 26, 2019

Keeping balance between flexibility and complexity is the hardest part of designing APIs. IMHO eZ\Publish\API\Repository\Values\ContentType\FieldDefintionCollection::filterByType(string $fieldTypeIdentifier): FieldDefinitionCollection method could be golden mean.

This covers all discussed use cases:

// 1. Check if field definition with given type exists
$contentType->getFieldDefinitions()->filterByType('ezuser')->isEmpty();
// 2. Get first field definition with given type
$contentType->getFieldDefinitions()->filterByType('ezuser')->first();
// 3. Get the number of fields definition of the given type
$contentType->getFieldDefinitions()->filterByType('ezimage')->count();
// 4. Get all fields definition of the given type
$contentType->getFieldDefinitions()->filterByType('ezimage');

and from the maintainer POV it will reduce the responsibility of \eZ\Publish\API\Repository\Values\ContentType\ContentType. Impl. of \eZ\Publish\API\Repository\Values\ContentType\FieldDefinitionCollection will takeover current † and future ‡ logic related to providing access field defintions:

namespace eZ\Publish\API\Repository\Values\ContentType;

use eZ\Publish\API\Repository\Values\ValueObject;
use eZ\Publish\SPI\Repository\Values\MultiLanguageDescription;
use eZ\Publish\SPI\Repository\Values\MultiLanguageName;

abstract class ContentType extends ValueObject implements MultiLanguageName, MultiLanguageDescription
{
    // ...

    abstract public function getFieldDefinitions(): FieldDefinitionCollection;

    public function getFieldDefinition(string $fieldDefinitionIdentifier): ?FieldDefinition
    {
        return $this->getFieldDefinitions()->get($fieldDefinitionIdentifier);
    }

    public function hasFieldDefinition(string $fieldDefinitionIdentifier): bool
    {
        return $this->getFieldDefinitions()->has($fieldDefinitionIdentifier);
    }
}

† See \eZ\Publish\Core\Repository\Values\ContentType\ContentType. There is a already quite lot of it.

‡ For example filtering field definitions by groups and is{Required,Translatable,Searchable} flags (might be useful for kernel internals)

Nevertheless we can also keep methods getFieldDefinitionsOfType and countFieldDefinitionsOfType and use
FieldDefintionCollection internally in the implementation of eZ\Publish\API\Repository\Values\ContentType\ContentType :-)

@adamwojs adamwojs force-pushed the enhanced_content_type_vo branch 3 times, most recently from 7068635 to 5325f75 Compare October 29, 2019 12:16
@@ -175,16 +175,67 @@ abstract class ContentType extends ValueObject implements MultiLanguageName, Mul
/**
* This method returns the content type field definitions from this type.
*
* @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition[]
* @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinitionCollection
Copy link
Contributor

Choose a reason for hiding this comment

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

As this goes to master we could skip @param/@return here and below

*
* @return \eZ\Publish\API\Repository\Values\ContentType\FieldDefinition|null
*/
public function get(string $fieldDefinitionIdentifier): ?FieldDefinition;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function get(string $fieldDefinitionIdentifier): ?FieldDefinition;
public function get(string $fieldDefinitionIdentifier): FieldDefinition;

*
* @return FieldDefinitionCollection A collection with the results of the filter operation.
*/
public function filter(Closure $p): FieldDefinitionCollection;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function filter(Closure $p): FieldDefinitionCollection;
public function filter(Closure $predicate): FieldDefinitionCollection;

*
* @param Closure $p The predicate used for filtering.
*
* @return FieldDefinitionCollection A collection with the results of the filter operation.
Copy link
Contributor

Choose a reason for hiding this comment

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

FQCN

protected function createFieldDefinitionCollectionMock(array $fieldDefinitions): FieldDefinitionCollection
{
return new \eZ\Publish\Core\Repository\Values\ContentType\FieldDefinitionCollection($fieldDefinitions);
// $collection = $this->createMock(FieldDefinitionCollection::class);
Copy link
Contributor

Choose a reason for hiding this comment

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

commented code

private $fieldDefinitions;

/**
* Field definitions indexed by identifier.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not convinced that this comment is necessary

};
}

public function offsetExists($offset)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
public function offsetExists($offset)
public function offsetExists($offset): bool

return isset($this->fieldDefinitions[$offset]);
}

public function offsetGet($offset)
Copy link
Contributor

Choose a reason for hiding this comment

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

missing return type

@alongosz alongosz requested a review from a team November 21, 2019 14:17
Copy link
Member

@alongosz alongosz left a comment

Choose a reason for hiding this comment

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

Would be good if someone from @ezsystems/documentation-team looked at PHPDocs.

Nitpicks:

*
* @param string $fieldTypeIdentifier
*
* @return FieldDefinitionCollection A collection with the results of the filter operation.
Copy link
Member

Choose a reason for hiding this comment

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

inconsistent PHPDoc: some param and return types are FQCN, some are not. E.g.: ^

@@ -144,7 +145,7 @@ public function testLoadVersionInfoById()
* Test for the loadVersionInfo() method, of a draft.
*
* @depends testLoadVersionInfoById
* @covers \eZ\Publish\Core\Repository\ContentService::loadVersionInfoById
* @covers \eZ\Publish\Core\Repository\ContentService::loadVersionInfoById
Copy link
Member

Choose a reason for hiding this comment

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

is it a result of CS fixer?


final class FieldDefinitionCollectionTest extends TestCase
{
public function testGet(): void
Copy link
Member

Choose a reason for hiding this comment

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

could you add @covers to all those methods?

Not insisting, because we do not use it right now, but it might come in handy at some point (I also expect it's beneficial IDE go to test nav - just guessing).

e.g.:

/**
 * @covers \eZ\Publish\API\Repository\Values\ContentType\FieldDefinitionCollection::get
 */

(intentionally no braces after the method name)

}

/**
* Returns predicate with is always true.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* Returns predicate with is always true.
* Returns a predicate with is always true.

not quite following "with" in this sentence, but seems like it's intentional?

};
}

public function offsetExists($offset)
Copy link
Member

Choose a reason for hiding this comment

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

no strict types?

@adamwojs adamwojs force-pushed the enhanced_content_type_vo branch from 5325f75 to b8dedac Compare December 19, 2019 09:57
@adamwojs
Copy link
Member Author

PR updated according to @alongosz and @mikadamczyk code review suggestions.

doc/bc/changes-8.0.md Outdated Show resolved Hide resolved
@adamwojs
Copy link
Member Author

Once again PR updated according to @mikadamczyk @alongosz code review suggestions :-)

@adamwojs adamwojs force-pushed the enhanced_content_type_vo branch from 5ecaf22 to 323fd60 Compare December 20, 2019 13:36
@adamwojs
Copy link
Member Author

PR rebased against current master to implement eZ\Publish\API\Repository\Exceptions\Exception on \eZ\Publish\API\Repository\Exceptions\OutOfBoundsException

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

7 participants