API Scaffolders can now be more expressive when registering fields #188

ScopeyNZ · 2018-10-06T06:13:34Z

This adds the ability to register a new FieldDefinition when adding fields with a scaffolder. This means you can explicitly specify the type AND the resolver for the field rather than relying on it's resolution through the field type of the field on the dataobject.

Part of the reasoning for this PR is it allows scaffolders to define types (ie. boolean, text, int) rather than using ->obj which falls back to "text" for everything that has no casting (or $db) defined. Eg:

$scaffolder->addField('MethodAccess', Type::bool(), function($obj) { 
     return $obj->myMethod();
}

This PR also tidies up how the fields are retained within the scaffolder, rather than being a non-associative array and then making sure the array is distinct on the field name, I updated it to just be associative by field name. Slight performance gain I suspect.

Fixes #162

Also fixes an issue where it was still asserting that fields existed on the dataobject when you might be using a custom resolver

ScopeyNZ · 2018-10-08T03:48:35Z

I feel like ArrayData is the wrong way to store field information. To have objects in an ArrayData it needs to extend ViewableData but why should a FieldDefinition extend ViewableData?

I'm pretty keen to work on making this more intuitive. I might be able to come up with a BC way to implement this before 4.3 but for now this is ready to go. Hopefully it's acceptable as is, although it feels slightly hacky with the $field->hasField('Definition') and $map = $field->toMap(); $map['Definition'] stuff.

unclecheese · 2018-10-16T01:23:36Z

I generally really like this. I think it's something we've badly needed for some time, but what's missing for me, and IMO blocking the merge, is a YAML counterpart to this, unless I'm missing it. Ultimately we want YAML to be the primary way of scaffolding, and the procedural approach as a backdoor.

ScopeyNZ · 2018-10-16T01:52:58Z

Sounds fair. I'll try and get some time to address this before Friday.

unclecheese · 2018-10-16T01:57:05Z

The fields array is already quite overloaded. You might want to break it out into something like casting, because there's a reasonable case for overloading the magic casting for native db fields, as well, e.g. an Int casted as Bool or vice versa.

Key being that for getters, casting is (pretty much) mandatory.

maxime-rainville

I said some mean things about the code.

I haven't tried running it locally yet. Let me know if you would like me to run it before addressing the feedback.

maxime-rainville · 2018-10-17T02:45:52Z

src/FieldDefinition.php

+use GraphQL\Type\Definition\Type;
+use SilverStripe\GraphQL\Scaffolding\Extensions\TypeCreatorExtension;
+
+class FieldDefinition


Can we include some docs about what the purpose of this class is?

I'm wondering if it would make sense to define a matching interface for this?

There's no unit test for this class.

maxime-rainville · 2018-10-17T02:47:12Z

src/FieldDefinition.php

+    /**
+     * @return string
+     */
+    public function getDescription()


Do we want matching setters for description, type and resolver?

maxime-rainville · 2018-10-17T02:51:46Z

src/Scaffolding/Scaffolders/DataObjectScaffolder.php

@@ -102,27 +104,38 @@ public function addFields(array $fieldData)
        foreach ($fieldData as $k => $data) {
            $assoc = !is_numeric($k);
            $name = $assoc ? $k : $data;
-            $field = ArrayData::create(
-                [
+


Do we want to update the example in the function doc to reflect that we now accept a FieldDefinition as well?

Let's use ArrayLib::is_assoc() for consistency.

maxime-rainville · 2018-10-17T02:56:43Z

src/Scaffolding/Scaffolders/DataObjectScaffolder.php

@@ -173,7 +186,9 @@ public function removeField($field)
     */
    public function removeFields(array $fields)
    {
-        $this->fields = $this->fields->exclude('Name', $fields);
+        foreach ($fields as $field) {


This looks like it only works off the key. Maybe update the doc to make it clear that it expect a list key names, otherwise some people might get confused and try to pass it a FieldDefinition object.

maxime-rainville · 2018-10-17T02:58:42Z

src/Scaffolding/Scaffolders/DataObjectScaffolder.php

@@ -211,7 +226,7 @@ public function getNestedQueries()
     */
    public function setFieldDescription($field, $description)
    {
-        $existing = $this->fields->find('Name', $field);
+        $existing = isset($this->fields[$field]);


Can we add a @throws statement in the doc?

maxime-rainville · 2018-10-17T03:22:34Z

src/Scaffolding/Scaffolders/DataObjectScaffolder.php

-                        $this->getDataObjectClass()
-                    )
-                );
+        foreach ($this->fields as $fieldName => $data) {


This method is getting kind of big and difficult to follow. Can we split it up into smaller private methods?

maxime-rainville · 2018-10-17T03:24:17Z

src/Scaffolding/StaticSchema.php

+     * @param ViewableData $instance
+     * @param $fieldName
+     */
+    public function assertValidFieldName(ViewableData $instance, $fieldName)


The entire point of this method is to throw an exception. We should include a @throws statement.

maxime-rainville · 2018-10-17T03:33:04Z

src/Scaffolding/Scaffolders/DataObjectScaffolder.php

-                [
+
+            // If we're given a field definition we can accept that interface
+            if ($data instanceof FieldDefinition) {


Can we update the unit test in tests/Scaffolding/Scaffolders/DataObjectScaffolderTest.php to test FieldDefinition?

maxime-rainville · 2018-10-17T03:33:41Z

src/Scaffolding/Scaffolders/DataObjectScaffolder.php

     * @return $this
     */
-    public function addField($field, $description = null)
+    public function addField($field, $definition = null)


Can we update the unit test in tests/Scaffolding/Scaffolders/DataObjectScaffolderTest.php to test FieldDefinition?

maxime-rainville · 2018-10-17T03:34:31Z

src/Scaffolding/Scaffolders/DataObjectScaffolder.php

@@ -645,6 +663,20 @@ protected function nestedConnections()
        return $queries;
    }

+    protected function assertValidFieldData($data, $fieldName)


Can we document this method?

unclecheese · 2018-10-17T03:50:00Z

src/Scaffolding/Scaffolders/DataObjectScaffolder.php

-                    'Description' => $description
-                ]
-            )
+        $this->fields[$field] = ArrayData::create(


I think this should be its own subclass of ArrayData. That way you can do your own validation, getters/setters, etc, and everything is more expressive.

unclecheese · 2018-10-17T03:51:16Z

src/Scaffolding/Scaffolders/DataObjectScaffolder.php

-                $target->addField($field->Name, $field->Description);
+                $arg = $field->Description;
+                // Work around `ArrayData` not supporting objects as values (unless they extend "ViewableData")
+                if ($field->hasField('Definition')) {


Seems to me you wouldn't have to do this hack if you created your own ArrayData subclass.

I agree. I don't even think it should be a subclass of ArrayData as it's quite opinionated around holding objects that extend ViewableData.

ScopeyNZ · 2018-10-17T04:01:43Z

Thanks for the reviews guys! I've blocked a couple of hours out tomorrow afternoon to run over this some more so I'll try to address everything then.

…ield definitions - addField now accepts only a FieldDefinition - scaffoldField can be used to auto-scaffold a FieldDefinition from just a field name - addFields has only been augmented to support FieldDefinitions, and scaffolding a FieldDefinition from an array of data - Yaml config can now be used to cast fields differently from how a DataObject normally casts it This can be useful if you want to expose a method rather than a field as you can define casting (rather than using the default string casting) - Cleaned up some code in the scaffolder

ScopeyNZ · 2018-10-18T04:56:41Z

I've added a commit - some details on the change are in the commit message.

I refactored this PR so that the only way that fields are maintained within DataObjectScaffolder is by using an associative array of FieldDefinitions. It occurred to me that this new object is only relevant for DataObjectScaffolder so I moved it a more relevant namespace.

I still need to increase the test coverage which I'll try to do before next week but I appreciate any comments you guys may have in the meantime.

ScopeyNZ · 2018-10-18T05:00:21Z

Also this is definitely breaking changes now :( - And isn't actually a dependency for the CCs elemental work in retrospect - We can drop it from the 4.3 release milestone (although it'd be awesome to get this into the new 3.0 release).

unclecheese · 2018-12-02T09:26:45Z

Does this pull request resolve #162 ? If so, can you connect the two?

ScopeyNZ · 2018-12-02T23:03:46Z

I guess it probably does. This has breaking changes though and it's not high on my list to address this right now.

ScopeyNZ · 2018-12-02T23:05:56Z

I don't have access to this repo for some reason so I can't link PR (nor edit the issue to mention a PR)

robbieaverill · 2018-12-02T23:07:31Z

I don't have access to this repo for some reason so I can't link PR (nor edit the issue to mention a PR)

Core team should, I've fixed that

maxime-rainville · 2018-12-16T22:45:05Z

@ScopeyNZ Do you still have work to do on this? We've got it in the blocked column on the OS board.

ScopeyNZ · 2018-12-16T22:46:47Z

I can't remember where I left it. Looks like there's at least a failed test to resolve. I'll put aside some time this week to look over it.

unclecheese · 2019-02-28T21:34:30Z

I hate to take this conversation in a totally different direction, but I'm wondering if, since it appears version 4 will use generated code from a dev task, maybe we should rely on statically analysed type hints for this?

Option A: Merge this into 3 only, with a new static analysis implementation in 4
Option B: Merge this into 3 and deprecate it in 4, supporting both for one major release cycle
Option C: Don't merge this. Let's just wait until 4, when we can do it right.
Option D: Merge this and support both long term (because maybe there's a legit case for having a GraphQL type that differs from your PHP type?)

ScopeyNZ · 2019-03-03T19:52:30Z

Considering that the API is changing so quickly and we're jumping to new majors so often I don't think it's worth increasing maintenance burden in the 3 branch. (Option C).

I still haven't found the time to run over this PR - it's not currently relevant for me.

maxime-rainville · 2019-03-03T21:46:13Z

Let's close for now. We can reopen later if we find it's relevant.

ScopeyNZ added 2 commits October 6, 2018 19:09

NEW Scaffolders can now be more expressive when registering fields

0dec09a

NEW Allow FieldDefinition to accept anything with TypeCreatorExtension

12dd829

Also fixes an issue where it was still asserting that fields existed on the dataobject when you might be using a custom resolver

ScopeyNZ force-pushed the pulls/3.0/field-flexibility branch 2 times, most recently from 4daa1ae to ef1167b Compare October 8, 2018 03:27

FIX Restoring the ArrayData method of storing fields for BC

e50694e

ScopeyNZ force-pushed the pulls/3.0/field-flexibility branch from ef1167b to e50694e Compare October 8, 2018 03:44

rupachup added this to the Recipe 4.3.0 milestone Oct 14, 2018

chillu assigned maxime-rainville Oct 16, 2018

maxime-rainville requested changes Oct 17, 2018

View reviewed changes

unclecheese reviewed Oct 17, 2018

View reviewed changes

ScopeyNZ changed the title ~~NEW Scaffolders can now be more expressive when registering fields~~ API Scaffolders can now be more expressive when registering fields Oct 18, 2018

chillu removed this from the Recipe 4.3.0 milestone Oct 18, 2018

unclecheese mentioned this pull request Dec 2, 2018

Type hinting support #162

Open

1 task

maxime-rainville assigned ScopeyNZ and unassigned maxime-rainville Dec 16, 2018

maxime-rainville closed this Mar 3, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

API Scaffolders can now be more expressive when registering fields #188

API Scaffolders can now be more expressive when registering fields #188

ScopeyNZ commented Oct 6, 2018 •

edited

Loading

ScopeyNZ commented Oct 8, 2018

unclecheese commented Oct 16, 2018

ScopeyNZ commented Oct 16, 2018

unclecheese commented Oct 16, 2018 •

edited

Loading

maxime-rainville left a comment

maxime-rainville Oct 17, 2018

maxime-rainville Oct 17, 2018

maxime-rainville Oct 17, 2018

maxime-rainville Oct 17, 2018

unclecheese Oct 17, 2018

maxime-rainville Oct 17, 2018

maxime-rainville Oct 17, 2018

maxime-rainville Oct 17, 2018

maxime-rainville Oct 17, 2018

maxime-rainville Oct 17, 2018

maxime-rainville Oct 17, 2018

maxime-rainville Oct 17, 2018

unclecheese Oct 17, 2018

unclecheese Oct 17, 2018

ScopeyNZ Oct 17, 2018

ScopeyNZ commented Oct 17, 2018

ScopeyNZ commented Oct 18, 2018

ScopeyNZ commented Oct 18, 2018

unclecheese commented Dec 2, 2018

ScopeyNZ commented Dec 2, 2018

ScopeyNZ commented Dec 2, 2018

robbieaverill commented Dec 2, 2018

maxime-rainville commented Dec 16, 2018

ScopeyNZ commented Dec 16, 2018

unclecheese commented Feb 28, 2019

ScopeyNZ commented Mar 3, 2019

maxime-rainville commented Mar 3, 2019

API Scaffolders can now be more expressive when registering fields #188

API Scaffolders can now be more expressive when registering fields #188

Conversation

ScopeyNZ commented Oct 6, 2018 • edited Loading

ScopeyNZ commented Oct 8, 2018

unclecheese commented Oct 16, 2018

ScopeyNZ commented Oct 16, 2018

unclecheese commented Oct 16, 2018 • edited Loading

maxime-rainville left a comment

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

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

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

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

ScopeyNZ commented Oct 17, 2018

ScopeyNZ commented Oct 18, 2018

ScopeyNZ commented Oct 18, 2018

unclecheese commented Dec 2, 2018

ScopeyNZ commented Dec 2, 2018

ScopeyNZ commented Dec 2, 2018

robbieaverill commented Dec 2, 2018

maxime-rainville commented Dec 16, 2018

ScopeyNZ commented Dec 16, 2018

unclecheese commented Feb 28, 2019

ScopeyNZ commented Mar 3, 2019

maxime-rainville commented Mar 3, 2019

ScopeyNZ commented Oct 6, 2018 •

edited

Loading

unclecheese commented Oct 16, 2018 •

edited

Loading