EZP-32261: Added field name override #98

mateuszbieniek · 2021-03-22T13:06:49Z

This PR adds an option to define field name overrides to avoid issues like:
https://issues.ibexa.co/browse/EZP-32261

src/Schema/Domain/Content/NameHelper.php

adamwojs · 2021-03-22T13:18:34Z

src/Resources/config/default_settings.yaml

+  ezplatform_graphql.schema.content.field_name.override:
+    id: id_


Is id the only one field which might generate name collision?

That's the only field that I'm aware of.

OK, but please rememeber ot document this behaviour. // cc @ezsystems/documentation-team

There are actually more, but they are all prefixed with _, making collisions less likely:

ezplatform-graphql/src/Resources/config/graphql/DomainContent.types.yaml

Lines 8 to 31 in 86f5870

_info:

description: "Item's content info"

type: Content

_content:

description: "Underlying content item"

type: Content

deprecationReason: "Renamed to _info"

_type:

description: "Item's content type"

type: ContentType

_location:

description: "The content's main location"

type: Location

_allLocations:

description: "All the content's locations"

type: "[Location]"

_name:

description: "The content item's name, in the prioritized language(s), based on the object name pattern"

type: String

_url:

description: "The content item's url alias, based on the main location."

type: String

_thumbnail:

type: Thumbnail

.

adamwojs · 2021-03-22T13:20:30Z

src/Schema/Domain/Content/NameHelper.php

+
+        // Workaround for https://issues.ibexa.co/browse/EZP-32261
+        if (\array_key_exists($fieldName, $this->fieldNameOverrides)) {
+            return $this->fieldNameOverrides[$fieldName];


IMHO is worth to warn user at this point that name collision happened (e.g. add warning to logs).

Added a warning level log entry so it will be shown in the output of

php bin/console ezplatform:graphql:generate-schema

Steveb-p

Ideally we would rather prevent declaring fields as "id", but I guess that's the next best thing.

I would suggest making it forbidden to do so in 4.x, treating "id" (along with uppercase variations) like a PHP keyword - just not allow it. WDYT?

mateuszbieniek · 2021-03-23T09:25:32Z

Ideally we would rather prevent declaring fields as "id", but I guess that's the next best thing.

I would suggest making it forbidden to do so in 4.x, treating "id" (along with uppercase variations) like a PHP keyword - just not allow it. WDYT?

I agree, but we still have to keep in mind, that we have to prepare some migration scenario for the customers upgrading from (for example) 2.5 LTS, where id was totally valid identifier for the fields.

And of course some entries in the documentation.

Steveb-p · 2021-03-23T09:35:52Z

src/Schema/Domain/Content/NameHelper.php

+    /**
+     * @var \Psr\Log\LoggerInterface
+     */
+    private $logger;


Use LoggerAwareTrait and LoggerAwareInterface instead.

Steveb-p · 2021-03-23T09:37:29Z

src/Schema/Domain/Content/NameHelper.php

+    public function __construct(LoggerInterface $logger, array $fieldNameOverrides)
    {
        $this->caseConverter = new CamelCaseToSnakeCaseNameConverter(null, false);
+        $this->logger = $logger;


Allow logger to be null and initialize it to NullLogger if done so. This will make the eventual testing easier.

Suggested change

public function __construct(LoggerInterface $logger, array $fieldNameOverrides)

{

$this->caseConverter = new CamelCaseToSnakeCaseNameConverter(null, false);

$this->logger = $logger;

public function __construct(array $fieldNameOverrides, ?LoggerInterface $logger = null)

{

$this->caseConverter = new CamelCaseToSnakeCaseNameConverter(null, false);

$this->logger = $logger ?? new NullLogger();

Don't forget to import the class if this suggestion is acceptable for you.

alongosz · 2021-03-23T11:36:23Z

src/Schema/Domain/Content/NameHelper.php

@@ -109,7 +138,7 @@ private function pluralize($name)
        }

        if (substr($name, -1) === 'y') {
-            if (in_array(substr($name, -2, 1), ['a', 'e', 'i', 'o', 'u'])) {
+            if (\in_array(substr($name, -2, 1), ['a', 'e', 'i', 'o', 'u'])) {


CS: you should rather import it with use function in_array;

alongosz · 2021-03-23T11:36:36Z

src/Schema/Domain/Content/NameHelper.php

@@ -124,7 +153,7 @@ private function pluralize($name)
            return substr($name, 0, -2) . 'i';
        }

-        if (in_array(substr($name, -2), ['on', 'um'])) {
+        if (\in_array(substr($name, -2), ['on', 'um'])) {


CS: the same remark as above

Running php-cs-fixer fix drops use function in_array; and adds \in_array 🤷‍♂️ - I guess if I push it, CS test will fail as well.

tomaszszopinski

QA approved on Ibexa DXP 3.2.5-dev & v3.2.5 with patch & diff.

lserwatka · 2021-03-25T13:53:15Z

Could you merge it up?

mateuszbieniek · 2021-03-25T14:02:36Z

Merged to 3.0 in 7d75c68

mateuszbieniek · 2021-03-25T14:14:11Z

Merged to master in 86f5870

Added field name override

84713fa

mateuszbieniek requested review from lserwatka, adamwojs, bdunogier and alongosz March 22, 2021 13:06

CS

70409bc

adamwojs reviewed Mar 22, 2021

View reviewed changes

adamwojs changed the title ~~Added field name override~~ EZP-32261: Added field name override Mar 22, 2021

Fixed tests + CR changes

18b3428

adamwojs added the Doc needed The changes require some documentation label Mar 23, 2021

Steveb-p approved these changes Mar 23, 2021

View reviewed changes

Added warning when a field name is overriden

87787d9

Steveb-p requested changes Mar 23, 2021

View reviewed changes

mateuszbieniek added 2 commits March 23, 2021 10:48

CR#2

8711af3

CR#2.1

7f8cd18

Steveb-p approved these changes Mar 23, 2021

View reviewed changes

adamwojs approved these changes Mar 23, 2021

View reviewed changes

alongosz approved these changes Mar 23, 2021

View reviewed changes

SylvainGuittard added the Ready for QA label Mar 24, 2021

tomaszszopinski self-assigned this Mar 25, 2021

tomaszszopinski approved these changes Mar 25, 2021

View reviewed changes

lserwatka merged commit 05d017d into ezsystems:2.2 Mar 25, 2021

tomaszszopinski added QA approved and removed Ready for QA labels Mar 25, 2021

mateuszbieniek deleted the ezp-32261_fix branch March 25, 2021 14:14

DominikaK removed the Doc needed The changes require some documentation label Apr 19, 2021

bdunogier mentioned this pull request Mar 3, 2022

Interface field Node.id expects type ID! but TemplateLookContent.id is type String bdunogier/ezplatform-graphql-bundle#89

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

EZP-32261: Added field name override #98

EZP-32261: Added field name override #98

mateuszbieniek commented Mar 22, 2021

adamwojs Mar 22, 2021

mateuszbieniek Mar 22, 2021

adamwojs Mar 23, 2021

bdunogier Apr 12, 2021

adamwojs Mar 22, 2021 •

edited

Loading

mateuszbieniek Mar 23, 2021

Steveb-p left a comment

mateuszbieniek commented Mar 23, 2021

Steveb-p Mar 23, 2021

Steveb-p Mar 23, 2021

alongosz Mar 23, 2021

alongosz Mar 23, 2021

mateuszbieniek Mar 23, 2021 •

edited

Loading

tomaszszopinski left a comment

lserwatka commented Mar 25, 2021

mateuszbieniek commented Mar 25, 2021

mateuszbieniek commented Mar 25, 2021

		ezplatform_graphql.schema.content.field_name.override:
		id: id_

	_info:
	description: "Item's content info"
	type: Content
	_content:
	description: "Underlying content item"
	type: Content
	deprecationReason: "Renamed to _info"
	_type:
	description: "Item's content type"
	type: ContentType
	_location:
	description: "The content's main location"
	type: Location
	_allLocations:
	description: "All the content's locations"
	type: "[Location]"
	_name:
	description: "The content item's name, in the prioritized language(s), based on the object name pattern"
	type: String
	_url:
	description: "The content item's url alias, based on the main location."
	type: String
	_thumbnail:
	type: Thumbnail

EZP-32261: Added field name override #98

EZP-32261: Added field name override #98

Conversation

mateuszbieniek commented Mar 22, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adamwojs Mar 22, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Steveb-p left a comment

Choose a reason for hiding this comment

mateuszbieniek commented Mar 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateuszbieniek Mar 23, 2021 • edited Loading

Choose a reason for hiding this comment

tomaszszopinski left a comment

Choose a reason for hiding this comment

lserwatka commented Mar 25, 2021

mateuszbieniek commented Mar 25, 2021

mateuszbieniek commented Mar 25, 2021

adamwojs Mar 22, 2021 •

edited

Loading

mateuszbieniek Mar 23, 2021 •

edited

Loading