Refactor SeoAwareContent

ElectricMaxxx commented 10 years ago

do no let the SeoAwareContent implement the Extractors too. Just let it implement SeoAwareInterface. Create some TestFixtures instead for testing that feature.

ElectricMaxxx commented 10 years ago

So i will do that this night. Then i will be able to build better examples in the sandbox https://github.com/symfony-cmf/cmf-sandbox/pull/243

dbu commented 10 years ago

there is the question if this content should extend the StaticContent (Model, not Doctrine\Phpcr!) from ContentBundle. this would be an optional dependency, as people can use this bundle without the provided concrete model class. the benefit would be to avoid the copy-paste with making menu referrers and additionalInfoBlocks and publish workflow working.

the drawback is that this creates a dependency on ContentBundle for cache:warmup and such...

so maybe we should really provide a minimal POC model here and explain in the doc how to extend the StaticContent for SEO.

ElectricMaxxx commented 10 years ago

As writen in the PR i would suggest to delete the Model complete and just provide the interfaces.

wouterj commented 10 years ago

I'm :+1: for removing the SeoAwareContent object completely, this bundle shouldn't provide something on the storage level imo

ElectricMaxxx commented 10 years ago

Yes. :+1:

ElectricMaxxx commented 10 years ago

So i tried my best, but i does not get the document removed. I got so many issues, i can't manage atm and there are so many other issues to do until tomorrow, that i will refactor the document only this night.

ElectricMaxxx commented 10 years ago

Added some text to #100

wouterj commented 10 years ago

you mean #94 ?

ElectricMaxxx commented 10 years ago

Yes.

ElectricMaxxx commented 10 years ago

So as a little conclusion for @WouterJ: The bundle provides two ways to come to a set of values in the SeoMetadata:

first and my Basic version ( but second sounds more Basic now): document implements SeoAwareInterface, has a SeoMetadata property which can be filled by the help of an sonata admin extension and a form type. This needs to be persisted as assoc(serialze from/to SeoMetadata needed)
second way: implementing extractor interfaces and reading values direct from the content document

Second works fine, first too if document has the callback, but this is a Big downside. Can't say a developer: please copy the callbacks. This was the reason to keep the SeoAwareContent, cause this had it implemented.

I would say: make two new branches after fixing the redirect bug. One for 1.0 where all persistence stuff is gone and one for forward work for 1.1 where the persistence test should stay. Or @dbu?

wouterj commented 10 years ago

I can see the problem. However, keeping SeoAwareContent or not doesn't fix the problem imo.

This bundle should be able to work with any document you use. It should not require to use the SeoAwareContent. For instance, the Page object from the SimpleCms implementing SeoAwareInterface should work fine too.

I also don't think that 1.0 should include features which we already know that we deprecate them in 1.1. I would prefer to move the release date of 1.0 a bit further to help us fix the problem instead.

So then about how we can fix this. Some ideas:

Create a new Doctrine Mapping Type special for dealing with SEO Metadata. This seems like the best option, but it looks like PHPCR-ODM doesn't support this?
Create a listener. Seems to be the best solution atm. What is the issue with it?

ElectricMaxxx commented 10 years ago

What about deleting that feature for 1.0. the bundle works fine without using the document or persisting the seoMetadata - just using the extractors. That is the Independent way you are talking about. So the feature to persist and edit the SeoMetadata can be a feature for a very early 1.1 or We wait with 1.0.

What does not work with the listener? I think i have pushed my current state to #94 if not i will do.

wouterj commented 10 years ago

For 1.0, there are 2 things to do afaics:

The redirection bug https://github.com/symfony-cmf/SeoBundle/issues/89
This problem

If you can push your current work on the listener to #94, I'll take a look at it this afternoon. You can then work on #89. If I don't get it correct before tonight, we'll shift the SeoAwareInterface and persisting thing to 1.1, otherwise we ship it in 1.0. What do you think?

ElectricMaxxx commented 10 years ago

I would like to do so, but i am on my way to my second Job :-( so Lets shifft it. I would suggest that you can do #83 its just a secure-if maybe with some reg-expr to compare current url with the one in the metadata. You have got request and SeoPresentation in the SeoContentListener. Just set the RedirectResponse only if the current and the destination one arn't same. Should be easy for you, and you have got a nicer sunday afternoon. I will refactor the Sandbox-Integration appr. at 10 pm. If Lukas is Asking.

A thing what you can help would be the tagging and branching for 1.0 but We can do it together in the evening. I am available by mail i hope.

wouterj commented 10 years ago

Ok, I'll take a look at #89 (because I think that's the one you meant). In the evening, there is a high change I'll be away till 10.30 PM (european time, GMT+1). So we can do the tagging then

ElectricMaxxx commented 10 years ago

I shouldn't referre by mail where there is no auto-fill. But you are right. Except the error, the PR Looks Fine.

Btw i had the same time change, hard. I made one our longer and slept one hour less.

wouterj commented 10 years ago

fixed now

symfony-cmf / seo-bundle

Refactor SeoAwareContent #83