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

[RFC] Deprecate extractors & introducing mappings #133

Open
wouterj opened this issue Apr 10, 2014 · 32 comments · May be fixed by #294
Open

[RFC] Deprecate extractors & introducing mappings #133

wouterj opened this issue Apr 10, 2014 · 32 comments · May be fixed by #294
Assignees
Milestone

Comments

@wouterj
Copy link
Member

wouterj commented Apr 10, 2014

Current Issues

We are having 4 issues with the current Extractors mechanism:

  1. The order in which they should be executed (create order/position property on extractors #107)
  2. SeoMetadata should inherit (create a helper to get the SeoMetadata decisions from child nodes #87)
  3. There should be a way to turn on/off extractors per document (do not enable title guesser by default for all #125)
  4. Some caching is needed for the extractors ([1.1] AnnotationExtractor and switching to JmsMetadata #118 (comment))

(1) and (3) are especially pretty hard to solve using the extractors.

I think we should go in a complete new direction for 1.1, however that can make things more complicated. I don't know if that's really welcome, since this bundle is pretty nice because it's not complicated.

Using Mappings

My idea was to use mappings instead of extractors. Instead of implementing some interfaces and introducing some wrapper methods which just call other functions, we should have a way to "map" some values of the document to SeoMetadata.

This can be done using the JmsSerializer. Some examples:

use Symfony\Cmf\Bundle\SeoBundle\Annotation as Seo;

class Page
{
    /**
     * @Seo\Title
     */
    protected $name;

    /**
     * @Seo\MetaKeywords({
     *     "strategy": "append"
     * })
     */
    protected $tags = array();

    /**
     * @Seo\MetaDescription(
     *     "stategy": "override"
     * })
     */
    protected $description;
}

Of course, we should also support Yaml and Xml mappings and mappings for methods. For instance:

Acme\BlogBundle\Document\Article:
    properties:
        title:
            title: ~
        tags:
            meta_keywords: { strategy: append }
        description:
            meta_description: { strategy: override }
<seo-mapping class="Acme\BlogBundle\Document\Article">
    <property name="title">
        <seo type="title" />
    </property>

    <property name="tags">
        <seo type="meta_keywords">
            <option name="strategy">append</option>
        </seo>
    </property>

    <property name="description">
        <seo type="meta_description">
            <option name="strategy">override</option>
        </seo>
    </property>
</seo-mapping>

Support Custom Data

Then we can also fix #131 by using a generic "Meta" mapping for custom meta values:

// ...
class Page
{
    /**
     * @Seo\Title
     * @Seo\Meta("og:title")
     */
    protected $name;
}
Acme\BlogBundle\Document\Article:
    properties:
        name:
            title: ~
            meta: { name: og:title }

Keeping BC

Of course, we should be BC. So we should keep supporting the extractors, but deprecate it.
replaces #118

@benglass
Copy link
Member

Containing all of the seo properties to a single document which can be attached to different phpcr document seems to have a few advantages over this approach.

  1. You dont have to define mappings repeatedly for title, keywords, etc.
  2. You can provide a single sonata admin class that manages this embedded SeoMetatada document
  3. You can add/remove seo fields on a single object instead of having to change mappings to any documents that might have seo data

@ElectricMaxxx
Copy link
Member

@benglass that a good point i haven't seen. What makes the decision not even easier ...

@wouterj
Copy link
Member Author

wouterj commented Apr 10, 2014

Hi @benglass, I don't fully support the idea, but I think it should be possible to integrate with the idea in this issue.

We can have a @SeoMetadata annotation, which can be used to specify that this property contains the SeoMetadata object.

Why I don't fully support your idea:

  • Most of the times, the Seo data will simply map directly to a property (title -> seo title, etc.)
  • Seo data should not be edited on another page than the content in Sonata admin
  • We then need a 1-1 mapping for SeoMetadata-Content, so I don't see the benefit of (3).

@ElectricMaxxx
Copy link
Member

@wouterj on disadvantage:
Atm we are able to extend existing objects to add the extractors. But with your proposal we would need to rewrite the object or ... this would be an advantage: we just add some xml-files for mapping.

@benglass
Copy link
Member

Regarding the strategy of append vs override I think this actually would need to be configurable on a per instance basis. For example you may generally want your title tags to prepend so that you have "Page | Section | Site" however you may need to specify that one some pages you just want to override the the title completely. I do agree that this inheritance/append/prepend control is an important feature.

@ElectricMaxxx
Copy link
Member

We have had this feature while creating the bundle but changed it to that translation key thing we have now. So the creation of title (i.e.) isn't visible anymore as you define Default title | %%content_title%%, but you are right this definition is global and can't be overwritten by any content.

@benglass
Copy link
Member

@wouterj regarding your proposed mappings approach, how would this handle arbitrary meta fields like robots, http-equiv etc. so they could be passed along to sonata page for rendering?

It seems we would also want a defined way to take an object that has seo data and transfer that to sonata page service for rendering (in a kernel listener on request, for example, if cmf main content is set then we would tell the page service about the metas from the loaded page).

@wouterj
Copy link
Member Author

wouterj commented Apr 10, 2014

It seems we would also want a defined way to take an object that has seo data and transfer that to sonata page service for rendering

That is exactly what this bundle does :) https://github.com/symfony-cmf/SeoBundle/blob/master/EventListener/SeoContentListener.php#L56

how would this handle arbitrary meta fields like robots, http-equiv etc. so they could be passed along to sonata page for rendering?

You mean, without a property that provides a value, but just a static value? Then it would be a constraint defined on the class

@benglass
Copy link
Member

@wouterj I'm specifically talking about arbitrary fields like robots, http-equiv and them being passed to the sonata page in the SeoPresentation class at https://github.com/symfony-cmf/SeoBundle/blob/master/Model/SeoPresentation.php#L145 which right now has the hard coded title, keywords, etc.

@benglass
Copy link
Member

@wouterj and I'm asking how the mapping approach you are advocating would allow for arbitrary metadata fields such as og:image, robots and http-equiv as opposed to having named annotations like @seo\Title which is hard coded to a property name

@ElectricMaxxx
Copy link
Member

Why not adding a field mapping with type and value attribbutes? Like @seo\Field(type="og:image")

@benglass
Copy link
Member

@ElectricMaxxx yes that is what I mean, we would need something like that if we are moving to a maping-based solution like @wouterj is proposing.

Another reason I think having things encapsulated into a separate SeoMetadata class may be a better approach is encapsulation. It prevents the class with the metadata from having to know the details about the metadata. I'm not sure I understand why 1-to-1 is bad. If you are worried about another hit to the database it could be implemented as a phpcr assoc with a getSeoMetadata getter (as you have) that just populates it from the persisted extras collection and returns an instance of SeoMetadata. It would be up to end users to decide whether they want to store the seo metadata as a separate document or create it on the fly in their document class from other class variables or an assoc variable.

@ElectricMaxxx
Copy link
Member

If i understand it right the mapping solution wouldn't change anything on the encapsulation of the SeoMetadata. Just the filling will be changed. Atm We have a direct property $seoMetadata (SeoAwareInteface) with a form type for admin support or the values will be extracted from content by an extractor pattern.
With that mapping the SeoMetadata as a encapsulated object will stay we will just extend it from a classMetada abstraction (doctrine/jmsmetadata) to fill the properties by the different mapping drivers.

So the basic properties like $title, $metaDescription, ... will stay and some extras will be added ($fields i.e.) - all with a coresponding mapping. Instead of filling the SeoMetadata by the extractors they would be filled by mapped properties/methods. We would be more flexible for scaleability and the SeoMetadata will stay with its interfaces.

@ElectricMaxxx
Copy link
Member

The more i think about it the more i belive that the mapping is almost equal to the extractor. Cause what does the driver? It calls a method (same as our defined ones) or fetches a properties value. instead of forcing the content to provide a getSeoTitle() method by an interface, the mapping just looks if there is a mapped "getter" for the title.
We will get a Lot more flexibility.

@benglass
Copy link
Member

@ElectricMaxxx so are you saying that a Page object which has seo metadata would have a getSeoMetadata method?

If not im wondering how you would be able to attach the sonata seo metadata admin form to it since it wouldnt have getters/setters for the SeoMetadata

@ElectricMaxxx
Copy link
Member

The form type admin support is available for SeoAwaInterface only. Which mean the class has to implement getter/setter for SeoMetadata. Have a look at the form type and the admin extension (writing from my mobile, so the links would be hard) for more information. An extractor solution has no admin support except the developer just extract properes 1-1 that have its own admin support.

To write an SeoAware admin support for mapped properties will be a little bit harder cause We won't have any fix properties to Point to.

@ElectricMaxxx
Copy link
Member

@wouterj at night i cam to the conclusion not to do that now. Lets keep that in mind for a 2.0 version. I think we shouldn't change everything now what took two month to work out. Lets expand the existing system on 1.* as long as it goes. Lets add an array for that arbitrary data to the SeoMetadata and create a form solution for that. I think we will need that time to figure out which values for the SeoMetadata are needed, which should be extendable which could be removed somehow. Lets give the bundle (and us) some time to learn. We have got several issues which wouldn't touch the SeoMetadata (#1, #132) which would be features to go into 1.1 alone. We would make more trouble/work then we would need.

@dbu @lsmith77 what do you think?

@wouterj
Copy link
Member Author

wouterj commented Apr 11, 2014

@ElectricMaxxx I have also thought about this tonight and I think we can do this in a BC way. So I agree with you, we should not worry about this for 1.0.0 anymore. Let's focus on some minor caching (will do that today) and usage of arbitrary data in SeoMetadata (can you do that in the coming days?).

Then we can start worrying about this for 1.1, 1.2 or a later 1.x release

@ElectricMaxxx
Copy link
Member

@wouterj we do not need to hurry with that BC break. As i wrote: lets wait an learn what other properties will come and wants to be handled. We will see.

@wouterj
Copy link
Member Author

wouterj commented Apr 11, 2014

@ElectricMaxxx there is no BC break :)

@ElectricMaxxx
Copy link
Member

So ... lets start in here ... @wouterj is watching the world cup atm, so i can do some stuff :-)

But first one question: Why JMSmetadata on not the DoctrineCommon stuff. In general both are doing the same (driver, factories classMetadata,..), but i think doctrine in more common and i have done some nice mappings with it:
https://github.com/ElectricMaxxx/DoctrineOrmOdmAdapter/tree/master/lib/Doctrine/ORM/ODMAdapter/Mapping
and would do the same. We should just think about some abstraction, which can be done to provide some metadata handling across all cmf bundles?

@lsmith77
Copy link
Member

lsmith77 commented Jul 9, 2014

jms metadata handles different formats automatically. as a result different formats behave consistently and supporting additional formats is less work.

@ElectricMaxxx
Copy link
Member

OK, you are right.
@lsmith77 you know if that lib got some mapping pass mechanism. I have got little stomach aches with static directories to put the mapping files (php, yml or xml) in and those bad long names. Or should we do that in configuration only?

@wouterj
Copy link
Member Author

wouterj commented Jul 9, 2014

@ElectricMaxxx I've already integrated jms into the RoutinAuto component. I think it's easier if I take care of this. I'm on a holiday currently, but when I'm back I'll start a PR.

@ElectricMaxxx
Copy link
Member

No worries i just created a new branch with the main classes. :-)

@ElectricMaxxx
Copy link
Member

What about Annotation Support? Would need an AnnotationReader-/Parser (Doctrine)?

@wouterj
Copy link
Member Author

wouterj commented Jul 10, 2014

Yes, jms only provide the domain layer of handling metadata. All implementation stuff (such as types of drivers) should come from other libraries.

@ElectricMaxxx
Copy link
Member

So i would say i leave it up to you @wouterj and i will have a look at the other (smaller) issue. I prepared a branch with some classes and defined the services. You just need to give them some life :-)

@wouterj
Copy link
Member Author

wouterj commented Aug 9, 2014

Let's skip this for 1.1

@ElectricMaxxx
Copy link
Member

👍

@ElectricMaxxx
Copy link
Member

I would say today, based on the discussion on irc: lets skip that to 2.0 as a major feature.

@wouterj
Copy link
Member Author

wouterj commented Nov 3, 2014

Yeah

@ElectricMaxxx ElectricMaxxx added this to the 2.0 milestone Feb 13, 2016
@wouterj wouterj linked a pull request May 27, 2016 that will close this issue
3 tasks
@ElectricMaxxx ElectricMaxxx modified the milestones: 2.1, 2.0 Nov 28, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants