Skip to content
This repository has been archived by the owner on Sep 16, 2021. It is now read-only.

Example for admin metadata implementation, #19

Merged
merged 7 commits into from
Jun 5, 2016
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
language: php

php:
- 5.4
- 5.5
- 5.6
- 7.0
Expand Down
63 changes: 63 additions & 0 deletions Description/Description.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
<?php

/*
* This file is part of the Symfony CMF package.
*
* (c) 2011-2015 Symfony CMF
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Cmf\Component\Resource\Description;

class Description
{
private $descriptors = [];
private $payloadType;

public function __construct($payloadType)
{
$this->payloadType = $payloadType;
}

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

Choose a reason for hiding this comment

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

Not "this" class.

*
* @param string $key
*
* @return mixed
Copy link
Member

Choose a reason for hiding this comment

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

can we be more specific what this is? how can the consumer work with the result?

Copy link
Member Author

Choose a reason for hiding this comment

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

well, the consumer can use the result in any way they please - for example the result could be the name of an .png file to use as an icon, or it could be the URL, or a vendor-assigned human title for the resource class (e.g. "Page"). Its pretty generic.

Copy link
Member

Choose a reason for hiding this comment

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

so you would split parts of the information into separate descriptions? like edit.html.icon and edit.html.url?

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, see the Descriptor class.

Then, when something needs a certain piece of information, it calls $description->has(Descriptor::LINK_HTML_EDIT).

*/
public function get($key)
{
if (!isset($this->descriptors[$key])) {
throw new \InvalidArgumentException(sprintf(
'Descriptor "%s" not supported for payload type "%s". Supported descriptors: "%s"',
$key,
$this->payloadType,
implode('", "', array_keys($this->descriptors))
));
}

return $this->descriptors[$key];
}

/**
* Set value for descriptors key.
*
* - It is possible to overwrite existing keys.
*
* - To help ensure interoperability, where possible, the key should be the
* value of one of the appropriate constants defined in the MetadescriptorsKey
Copy link
Member Author

Choose a reason for hiding this comment

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

in the Descriptor class

* class.
*
* @param string $key
* @param mixed $value
*/
public function set($key, $value)
{
$this->descriptors[$key] = $value;
}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess that would be merge() then.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

@dbu dbu May 27, 2016 via email

Choose a reason for hiding this comment

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

}
36 changes: 36 additions & 0 deletions Description/DescriptionEnricherInterface.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
<?php

/*
* This file is part of the Symfony CMF package.
*
* (c) 2011-2015 Symfony CMF
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Cmf\Component\Resource\Description;

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

interface DescriptionEnricherInterface
Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

{
/**
* Enrich the payload description.
*
* @param Description $description
* @param object $payload
*
* @return Description
*/
public function enrich(Description $description, $payload);

/**
* Return true if the provider supports the given type.
*
* @param CmfResource $resource
*
* @return bool
*/
public function supports(CmfResource $resource);
}
49 changes: 49 additions & 0 deletions Description/DescriptionFactory.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<?php

/*
* This file is part of the Symfony CMF package.
*
* (c) 2011-2015 Symfony CMF
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Cmf\Component\Resource\Description;

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

class DescriptionFactory
{
private $enrichers = [];

/**
* @param array $enrichers
*/
public function __construct(array $enrichers)
{
$this->enrichers = $enrichers;
}

/**
* Return a description of the given (CMF) Resource.
*
* @param CmfResource $resource
*/
Copy link
Member

Choose a reason for hiding this comment

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

missing @return

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

@wouterj wouterj May 24, 2016

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

$description = new Description($type);

foreach ($this->enrichers as $enricher) {
if (false === $enricher->supports($resource)) {
continue;
}

$enricher->enrich($description, $payload);
}

return $description;
}
}
52 changes: 52 additions & 0 deletions Description/Descriptor.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,52 @@
<?php

/*
* This file is part of the Symfony CMF package.
*
* (c) 2011-2015 Symfony CMF
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Cmf\Component\Resource\Description;

/**
* Class containing standard description keys which should be used when appropriate
* by enrichers in order to provide a level of interoperability.
*/
final class Descriptor
{
/**
* Alias for the resource type for example `app.page`.
*/
const TYPE_ALIAS = 'type.alias';

/**
* Humanized representation of the resource type.
*/
const TYPE_TITLE = 'type.title';

/**
* Title of the actual payload, e.g. "My Blog Post".
*/
const PAYLOAD_TITLE = 'title';

/**
* Keys to be used to store link values for HTML.
Copy link
Member Author

@dantleech dantleech May 25, 2016

Choose a reason for hiding this comment

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

Refer to these as descriptors, not "keys"

*/
const LINK_EDIT_HTML = 'link.edit.html';
const LINK_CREATE_HTML = 'link.create.html';
const LINK_UPDATE_HTML = 'link.update.html';
const LINK_REMOVE_HTML = 'link.remove.html';
const LINK_SHOW_HTML = 'link.show.html';

/**
* Keys to be used to store link values for REST.
*/
const LINK_EDIT_REST = 'link.edit.rest';
const LINK_CREATE_REST = 'link.create.rest';
const LINK_UPDATE_REST = 'link.update.rest';
const LINK_REMOVE_REST = 'link.remove.rest';
const LINK_SHOW_REST = 'link.show.rest';
}
97 changes: 97 additions & 0 deletions Tests/Unit/Description/DescriptionFactoryTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
<?php

/*
* This file is part of the Symfony CMF package.
*
* (c) 2011-2015 Symfony CMF
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Cmf\Component\Resource\Tests\Unit\Repository;

use Symfony\Cmf\Component\Resource\Description\Description;
use Symfony\Cmf\Component\Resource\Description\DescriptionEnricherInterface;
use Prophecy\Argument;
use Symfony\Cmf\Component\Resource\Description\DescriptionFactory;
use Symfony\Cmf\Component\Resource\Repository\Resource\CmfResource;

class DescriptionFactoryTest extends \PHPUnit_Framework_TestCase
{
private $factory;
private $enricher1;
private $enricher2;
private $payload;
private $resource;

public function setUp()
{
$this->enricher1 = $this->prophesize(DescriptionEnricherInterface::class);
$this->enricher2 = $this->prophesize(DescriptionEnricherInterface::class);
$this->resource = $this->prophesize(CmfResource::class);

$this->payload = new \stdClass();
$this->resource->getPayload()->willReturn($this->payload);
$this->resource->getPayloadType()->willReturn('payload-type');
}

/**
* It should return an enriched description.
*/
public function testGetPayloadDescription()
{
$this->enricher1->enrich(Argument::type(Description::class), $this->payload)
->will(function ($args) {
$description = $args[0];
$description->set('foobar', 'barfoo');
});
$this->enricher1->supports($this->resource->reveal())->willReturn(true);
$this->enricher2->enrich(Argument::type(Description::class), $this->payload)
->will(function ($args) {
$description = $args[0];
$description->set('barfoo', 'foobar');
});
$this->enricher2->supports($this->resource->reveal())->willReturn(true);

$description = $this->createFactory([
$this->enricher1->reveal(),
$this->enricher2->reveal(),
])->getPayloadDescriptionFor($this->resource->reveal());

$this->assertInstanceOf(Description::class, $description);
$this->assertEquals('barfoo', $description->get('foobar'));
$this->assertEquals('foobar', $description->get('barfoo'));
}

/**
* It should ignore providers that do not support the payload type.
*/
public function testIgnoreNonSupporters()
{
$this->enricher1->enrich(Argument::cetera())->shouldNotBeCalled();
$this->enricher1->supports($this->resource->reveal())->willReturn(false);

$this->enricher2->enrich(Argument::cetera())->shouldBeCalled();
$this->enricher2->supports($this->resource->reveal())->willReturn(true);

$this->createFactory([
$this->enricher1->reveal(),
$this->enricher2->reveal(),
])->getPayloadDescriptionFor($this->resource->reveal());
}

/**
* It should work when no enrichers are given.
*/
public function testNoEnrichers()
{
$description = $this->createFactory([])->getPayloadDescriptionFor($this->resource->reveal());
$this->assertInstanceOf(Description::class, $description);
}

private function createFactory(array $enrichers)
{
return new DescriptionFactory($enrichers);
}
}
53 changes: 53 additions & 0 deletions Tests/Unit/Description/DescriptionTest.php
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
<?php

/*
* This file is part of the Symfony CMF package.
*
* (c) 2011-2015 Symfony CMF
*
* For the full copyright and license information, please view the LICENSE
* file that was distributed with this source code.
*/

namespace Symfony\Cmf\Component\Resource\Tests\Unit\Repository;

use Symfony\Cmf\Component\Resource\Description\Description;
use Symfony\Cmf\Component\Resource\Description\Descriptor;

class DescriptionTest extends \PHPUnit_Framework_TestCase
{
/**
* @var Description
*/
private $description;

public function setUp()
{
$this->description = new Description('some-type');
}

/**
* It should allow values to be set and retrieved.
*/
public function testGetSet()
{
$this->description->set(Descriptor::TYPE_ALIAS, 'page');
$this->description->set(Descriptor::LINK_EDIT_HTML, '/path/to/edit');
$this->description->set('custom.key', 'Hello');

$this->assertEquals('page', $this->description->get(Descriptor::TYPE_ALIAS));
}

/**
* It should throw an exception when requesting an unsupported descriptor.
*
* @expectedException \InvalidArgumentException
* @expectedExceptionMessage Descriptor "not there" not supported for payload type "some-type". Supported descriptors: "foo", "bar"
*/
public function testGetUnsupported()
{
$this->description->set('foo', 'bar');
$this->description->set('bar', 'foo');
$this->description->get('not there');
}
}