Example for admin metadata implementation, #19

dantleech · 2016-05-23T10:18:46Z

Descriptors

A description is just an key / value store and passed to any number of
supprorting "enrichers" (e.g. SonataEnricher, SyliusEnricher) which add any
information.

In order to ensure interoperability a Descriptor class is given which
provides constant values for use as descriptor keys, e.g. LINK_EDIT_HTML,
TYPE_ALIAS, TITLE.

Use is as follows:

$factory = new DescriptionFactory([
    new SyliusEnricher(...),  // add links for Sylius
    new IconEnricher()        // add "icon" URLs (as an example)
]); 

$resource = $registry->get('/pages/foobar/my-article'); // get a resource
$description = $factory->getPayloadDescriptionFor($resource);

echo $description->get(Descriptor::TITLE); // "My Article"
echo $description->get(Descriptor::TYPE); // AcmeBundle\Document\Article
echo $description->has(Descriptor::LINK_HTML_EDIT); // true
echo $description->get(Descriptor::LINK_HTML_EDIT); // /path/to/edit/page/123

Enhancers

Enhancers included in this PR:

SonataAdminEnhancer: Adds CRUD URLs for resources.
SyliusEnhancer: Adds CRUD URLs for resources.

ElectricMaxxx · 2016-05-23T21:12:42Z

Metadata/PayloadMetadata.php

+     */
+    public function get($key)
+    {
+        if (!isset($this->supported[$key])) {


Where would you fill that array?

That was just junk from the WIP.

dantleech · 2016-05-24T14:00:49Z

Description/DescriptionEnricherInterface.php

+
+use Symfony\Cmf\Component\Resource\Repository\Resource\CmfResource;
+
+interface DescriptionEnricherInterface


Consider renaming this with the more apt term (imo) "augmenter"

Or use Enhancer as with the RoutingBundle - I think the semantic is the same /cc @dbu

wouterj · 2016-05-24T21:15:36Z

Description/DescriptionFactory.php

+    public function getPayloadDescriptionFor(CmfResource $resource)
+    {
+        $type = $resource->getPayloadType();
+        $payload = $resource->getPayload();


we have to check if payload is an object. At the moment, it can be anything: https://github.com/symfony-cmf/resource/blob/master/Repository/Resource/CmfResource.php#L40-L48 (I even believe we use it as string in the ResourceBundle atm)

That was a wrong comment. However, I don't see why we have to know the payload? Removing this and injecting the resource in the enricher would allow us to use any puli resource.

In that case we would need to inject tge Resource into the Description, making it a Resource Description and not a Payload Description (what I see it as now) but, what you suggest probably makes more sense.

Updated, the Description now accepts a PuliResource and we pass only the Description to the enhance (renamed from enrich) method of the Enhancer.

dantleech · 2016-05-25T08:48:20Z

Description/Description.php

+    /**
+     * Return the descriptors value for the given key.
+     *
+     * The key should be one of the constants defined in this class.


Not "this" class.

wouterj · 2016-05-25T08:59:58Z

Description/Description.php

+    public function set($key, $value)
+    {
+        $this->descriptors[$key] = $value;
+    }


What about also implementing the other methods mentioned in http://symfony.com/doc/current/contributing/code/conventions.html#method-names ?

Yeah we are missing has() and possibly all().

something to set several descriptors in one call, by providing a hashmap that is merged with $this->descriptors?

I guess that would be merge() then.

or setDescriptor($key, $value) and setDescriptors($descriptors)

But would setDescriptors replace all the descriptors or merge them? I would assume without prior knowledge that it would replace them.

ah, true. maybe addDescriptors? mergeDescriptors feels strange. that adding something that already exists overwrites the existing value seems no big surprise to me, and we can state it explicitly in the phpdoc

I prefer merge, as that correlates with array_merge which is what you are actually doing. However, I think the ParameterBag uses add() with the same semantic.

I guess suffixing Descriptor makes sense.

your call :-)

wouterj · 2016-05-25T09:03:39Z

Although I understand the reasons behind the Descriptor constants, I'm afraid blocks the current limits the amount of options that enhancers can edit.

Also, why not add these constants to the Description class?

dantleech · 2016-05-25T09:51:17Z

Although I understand the reasons behind the Descriptor constants, I'm afraid blocks the current limits the amount of options that enhancers can edit.

My main concern is to try and avoid a situation where one implementation uses a different key/descriptor than another implementation for the same thing.

But I don't really see any alternative to defining a canonical set of "descriptors". How else could we "solve" this problem?

Also, why not add these constants to the Description class?

It just seemed a little bit more semantic:

$description->set(Descriptor::TITLE, 'Hello');
$description->set(Descriptor::LINK_EDIT_HTML, 'http://foo/bar.html');

vs.

$description->set(Description::TITLE, 'Hello');
$description->set(Description::LINK_EDIT_HTML, 'http://foo/bar.html');

and it separated the responsiblitiy of storing these values into a separate
class which just seems tidier.

Don't feel too strongly about it however.

dbu · 2016-05-26T06:31:52Z

Description/Description.php

+class Description
+{
+    /**
+     * @var array


what are the values in this array? strings, or something more complex? a list of label => url?

I think we can limit them to be scalar values. If there is a use case for array values we could allow that later.

We should allow arrays also, f.e. to indicate the valid child payload types.

dantleech · 2016-05-29T09:24:02Z

Description/DescriptionEnhancerInterface.php

+    /**
+     * Return true if the provider supports the given type.
+     *
+     * @param CmfResource $resource


dantleech · 2016-06-05T09:46:49Z

@wouterj could you have a look, I anticipate making further changes but I think it would be good to get this in to facilitate further work.

wouterj · 2016-06-05T12:05:27Z

If everything works as expected from your point of view, let's just merge. We can improve it while it's in the master branch.

I agree with the concept behind this PR and don't see major problems in a quick review. I don't have time to test it atm.

dantleech added the wip/poc label May 23, 2016

Admin metadata

fdd8ff6

dantleech force-pushed the admin_metadata branch from 104b973 to fdd8ff6 Compare May 23, 2016 10:19

ElectricMaxxx reviewed May 23, 2016
View reviewed changes

dantleech force-pushed the admin_metadata branch 2 times, most recently from 16c5c92 to 719c836 Compare May 24, 2016 13:59

dantleech reviewed May 24, 2016
View reviewed changes

Refactored

5cab7c0

dantleech force-pushed the admin_metadata branch from 719c836 to 5cab7c0 Compare May 24, 2016 14:03

Removed 5.4 support

624f650

wouterj reviewed May 24, 2016
View reviewed changes

dantleech force-pushed the admin_metadata branch from 02bf42b to d4ff120 Compare May 25, 2016 08:45

dantleech changed the title ~~[RFC] Example for admin metadata implementation,~~ Example for admin metadata implementation, May 25, 2016

dantleech reviewed May 25, 2016
View reviewed changes

dantleech force-pushed the admin_metadata branch 2 times, most recently from 9932069 to b11a77c Compare May 25, 2016 08:55

Allow PuliResources

dc64748

dantleech force-pushed the admin_metadata branch from b11a77c to dc64748 Compare May 25, 2016 08:56

wouterj reviewed May 25, 2016
View reviewed changes

dantleech mentioned this pull request May 25, 2016

Standardize the retrieval of URLs relating to a specific resource. #18

Closed

dbu reviewed May 26, 2016
View reviewed changes

dantleech force-pushed the admin_metadata branch 2 times, most recently from ba30ba0 to 0b75002 Compare May 27, 2016 09:07

Adding enhancers

46b3669

dantleech force-pushed the admin_metadata branch from 0b75002 to 46b3669 Compare May 27, 2016 09:08

dantleech mentioned this pull request May 27, 2016

Support for enhancers symfony-cmf/resource-bundle#26

Closed

dantleech force-pushed the admin_metadata branch from 4df78ef to 3290233 Compare May 29, 2016 09:13

Added Sylius Resource Enhancer

9289b67

dantleech force-pushed the admin_metadata branch from 3290233 to 9289b67 Compare May 29, 2016 09:23

dantleech reviewed May 29, 2016
View reviewed changes

dantleech force-pushed the admin_metadata branch from 817ff07 to 9289b67 Compare May 31, 2016 08:03

This was referenced Jun 4, 2016

Move SonataEnhancer to SonataAdminBundle symfony-cmf/resource-rest-bundle#30

Closed

Support the Resource Descriptions (remove Enhancers from here). symfony-cmf/resource-rest-bundle#35

Closed

Fixes

c9b17b6

dantleech merged commit b676f3c into master Jun 5, 2016

dantleech deleted the admin_metadata branch June 5, 2016 12:13

dantleech removed the wip/poc label Jun 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Example for admin metadata implementation, #19

Example for admin metadata implementation, #19

dantleech commented May 23, 2016 •

edited

Loading

ElectricMaxxx May 23, 2016

dantleech May 24, 2016

dantleech May 24, 2016

dantleech May 25, 2016

wouterj May 24, 2016 •

edited

Loading

dantleech May 25, 2016

dantleech May 25, 2016

dantleech May 25, 2016

wouterj May 25, 2016

dantleech May 25, 2016

dbu May 26, 2016

dantleech May 26, 2016

dbu May 26, 2016

dantleech May 27, 2016

dbu May 27, 2016

dantleech May 27, 2016

dbu May 27, 2016 via email

wouterj commented May 25, 2016 •

edited

Loading

dantleech commented May 25, 2016

dbu May 26, 2016

dantleech May 26, 2016

dantleech Jun 5, 2016

dantleech Jun 5, 2016

dantleech May 29, 2016

dantleech commented Jun 5, 2016

wouterj commented Jun 5, 2016


		use Symfony\Cmf\Component\Resource\Repository\Resource\CmfResource;

		interface DescriptionEnricherInterface

Example for admin metadata implementation, #19

Example for admin metadata implementation, #19

Conversation

dantleech commented May 23, 2016 • edited Loading

Descriptors

Enhancers

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

wouterj May 24, 2016 • edited Loading

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

dbu May 27, 2016 via email

Choose a reason for hiding this comment

wouterj commented May 25, 2016 • edited Loading

dantleech commented May 25, 2016

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

dantleech commented Jun 5, 2016

wouterj commented Jun 5, 2016

dantleech commented May 23, 2016 •

edited

Loading

wouterj May 24, 2016 •

edited

Loading

wouterj commented May 25, 2016 •

edited

Loading