Version upgrades and cleanups #58

briancappello · 2014-12-04T14:17:51Z

This PR aims to bring the BlogBundle more in line with the CMF Bundle Standards.

General Changes

Updated dependencies to CMF 1.2.*
Updated routing configuration to work with RoutingAutoBundle 1.0.*
Parameter and service ID names have been changed to match the conventions laid out in the standards
DI in general got cleaned up with the intent of making it easier for 3rd party bundles to extend the BlogBundle

Persistence

Blog and Post have been refactored into the Model and Doctrine/Phpcr namespaces.
Their PHPCR mapping files were likewise relocated into Resources/config/doctrine-phpcr
The mapping files are now auto-loaded by a DoctrinePhpcrMappingsPass, if the class is found
Added a GenericInitializer service to create the cmf_blog.blog_basepath if it's not found
Rewrote the PostRepository to support searching by an array input-verified with the OptionsResolver

Front End

Separated BlogController into BlogController and PostController
Templates were moved, renamed and refactored accordingly. Added markup for easier styling (still on BS2)

New Features

Added (optional) route to list blogs
Added (optional) pagination support, using the KnpPaginatorBundle
Added (optional) PHPCR data fixture to ease development

Known Issues

Make sure configuration is loadable when defined as XML
The XML schema file for the config is out-of-date with Configuration
Translatable content. It might work; untested.

Addresses issues #53 and #57

briancappello · 2014-12-04T14:20:14Z

Resources/config/admin.xml

-                group="dashboard.group_blog" 
-                label_catalogue="CmfBlogBundle" 
-                label="dashboard.label_blog" 
+        <service id="cmf_blog.blog_admin" class="%cmf_blog.blog_admin_class%">


parameter name BC break

lsmith77 · 2014-12-04T15:05:42Z

awesome that you are working on this! in general I would say that you should not concern yourself with BC too much as this was never stable.

briancappello · 2014-12-04T15:07:15Z

Admin/PostAdmin.php

+                ))
+                ->add('body', 'textarea')
+                ->add('blog', 'phpcr_document', array(
+                    'class' => 'Symfony\Cmf\Bundle\BlogBundle\Document\Blog',


do not hard-code this

…o Doctrine/Phpcr

briancappello · 2014-12-11T22:40:20Z

Repository/PostRepository.php

+                            )
+                        );
+                    }
+                };


This code smells, but I'm not sure how best to improve it.
$allowedFields should not be hard-coded, and $throwIfNotFound isn't testable/reusable (eg if somebody wanted to add 'groupBy' support)

Why do you even need to validate this here? The PHPCR-ODM will throw an exception if you try and order by an invalid field. Given that, do you even need an option resolver?

Maybe just limit the type and let the PHPCR-ODM validate the value.

…tion.......

dantleech · 2014-12-12T06:25:29Z

Controller/PostController.php

+
+        return $this->renderResponse($contentTemplate, compact('post'));
+    }
+


Extra blank line

dantleech · 2014-12-12T06:44:11Z

Looking really nice :)

Tagging was an issue for me here -- i.e. I started by writing Tag functionality in the bundle, but it shouldn't be the responsiblity of this bundle at all. i.e. it should be a CMF component which handles tags (or if possible something from another vendor, e.g. Sylius etc).

For my blog I wrote a tagging bundle specifically for PHPCR-ODM, but I won't vouch for its quality/methodology and don't want to maintain it :P

dbu · 2014-12-12T06:59:10Z

for tagging, i looked into SyliusTaxonomy but never found time to wrap it up: Sylius/Sylius#647

i suggest to leave tags out completely of this PR for now and instead check the sylius option again and then integrate here.

dantleech · 2014-12-13T17:29:51Z

It would also be neat to add functional tests to this bundle as we have done for the others. We have a testing component which lets you easily build a test application.

The idea is that we have almost a "demo" application which you can run as follows:

$ ./vendor/symfony-cmf/testing/bin/server

Functional tests can then be added like in this example from the MenuBundle.

I can help you out here and it needn't be merged with this PR.

briancappello · 2014-12-22T17:15:09Z

@dantleech Thanks for taking the time to review this PR! I went ahead and removed all of the tagging-related code for now. I also added a functional test for the BlogAdmin create action. After reviewing the functional test example in the MenuBundle, I wasn't entirely clear on the precise difference between an integration test and more of a web test. I went with the latter, as this is how we've been handling functional testing internally at VDW. Any feedback you have on methodology in that regard would be appreciated!

dantleech · 2014-12-22T17:38:40Z

.gitignore

@@ -1,4 +1,7 @@
 Tests/Resources/app/cache
 Tests/Resources/app/logs
+Tests/Resources/test_db.sqlite


You could also put this in app/cache

dantleech · 2014-12-22T18:07:44Z

I wasn't entirely clear on the precise difference between an integration test and more of a web test.

The web tests test are HTTP tests, integration tests would be things that test services from the container
directly. So the WebTests are the right thing here.

The idea of the web test is that we should be able to test an implentation of the BlogBundle, i.e. the
Kernel in Tests/Resources/app/AppKernel should be treated as a working web application. (as I may have
mentioned before it is even possible to run a webserver using the vendor/symfony-cmf/testing/bin/server
script)

For example, in addition to the admin, we should test viewing a post via the
BlogController.

dbu · 2014-12-26T09:53:10Z

Repository/PostRepository.php

-use Symfony\Cmf\Bundle\BlogBundle\Document\Post;
+use Doctrine\ODM\PHPCR\Mapping\ClassMetadata;
+use Symfony\Component\OptionsResolver\OptionsResolver;
+use Symfony\Cmf\Bundle\BlogBundle\Doctrine\Phpcr\Post;

 class PostRepository extends DocumentRepository


@dantleech how do you feel about the repositories. it seems correct to do this, but half of this is not blog specific - and afaik we did not do it in other bundles. i wonder if we should provide the base for most of this in the ContentBundle?

i am a bit unsure about the options resolver things we do. in the end, we wrap the query builder some more...

The search method seems only to be called to find the posts which belong to a particular blog.

In which case I would recommend removing it and replacing it with a findForBlog method if required -- and I am not sure it is as we could switch to the PageFanta paginator and its native PHPCR-ODM "driver".

So basically .. I think we could remove this, what do you think @briancappello ?

I would be fine with removing this repository class and switching to PagerFanta

maybe lets do this in another PR after this one is merged?

lsmith77 · 2014-12-26T11:05:39Z

didn't review this PR .. but I wonder if we shouldn't just merge at some point. then we can more easily collaborate on the details to tweak.

dantleech · 2014-12-26T12:30:26Z

Repository/PostRepository.php

+     * @param string $title
+     * @return Post|null
+     */
+    public function findByTitle($title)


Where does this get called?

I don't think it's used - no objections on my end to removing it

lsmith77 · 2015-01-21T10:52:34Z

composer.json

        "symfony-cmf/routing-auto-bundle": "1.0.*",
-        "doctrine/phpcr-bundle": "1.0.*",
-        "doctrine/phpcr-odm": "1.0.*"
+        "sonata-project/doctrine-extensions": "~1.0.2",


should this be ~1.0.2|1.1 ?

or ~1.0,>=1.0.2 ?

lsmith77 · 2015-02-06T10:29:48Z

ping

jrmyio · 2015-03-22T20:10:28Z

any update on this?

In oct. 2014 briancappello asked for updated deps, in december he did all the work, in march 2015 still nothing was merged. Why is this bundle even here if nobody maintains it and any help gets ignored.

im trying out symfony cmf but this is really discouraging

wouterj · 2015-03-22T20:58:26Z

@connxnl keep in mind that this bundle is one of the showcase bundles of the CMF. It's goal is not so much of providing functionality, but more of providing examples of how to combine multiple CMF bundles.

Seems like this PR is ready to be merged, but we didn't get a notice of the update (maintainers only get a notice about comments). So thanks for your head up!

dbu · 2015-03-23T07:25:49Z

.gitignore

 composer.lock
-vendor
+vendor/
+.idea


this is the responsibility of the global git configuration of the developer, it is not specific to the project but to specific development environments. please remove this line.

dbu · 2015-03-23T07:29:15Z

@dantleech can you check if you see any blockers in here? i can do some minimal cleanup, squash the commits and merge.

dantleech · 2015-03-23T07:33:24Z

Controller/BaseController.php

+/**
+ * Base Controller
+ *
+ * @author Daniel Leech <[email protected]>


I didn't write this :)

dbu · 2015-03-25T22:10:25Z

cleaned up and created a new branch in this repo

Brian added 2 commits November 30, 2014 12:48

update composer.json dep versions

6666e4b

cleanups

0dd64f5

briancappello reviewed Dec 4, 2014
View reviewed changes

briancappello force-pushed the version_upgrades branch from c2816bf to 0dd64f5 Compare December 5, 2014 16:00

Brian Cappello added 3 commits December 5, 2014 20:09

refactor Blog and Post into Models and move the document entities int…

18747ee

…o Doctrine/Phpcr

front end refactoring - repositories, controllers and templates

899e53e

refactor configuration

5c11169

briancappello reviewed Dec 11, 2014
View reviewed changes

it helps to update the bundle extension when you change the configura…

ac7955a

…tion.......

dantleech reviewed Dec 12, 2014
View reviewed changes

Controller/PostController.php

return $this->renderResponse($contentTemplate, compact('post'));

}

Copy link

Member

dantleech Dec 12, 2014

Choose a reason for hiding this comment

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

Extra blank line

Brian Cappello added 2 commits December 12, 2014 16:03

remove everything tag related

948cb7c

style consistency

59aec76

add test for blog admin create action

2d12503

dantleech reviewed Dec 22, 2014
View reviewed changes

dbu reviewed Dec 26, 2014
View reviewed changes

dantleech reviewed Dec 26, 2014
View reviewed changes

remove superfluous code

27bb8a4

lsmith77 added the in progress label Jan 21, 2015

lsmith77 reviewed Jan 21, 2015
View reviewed changes

convert routing auto config to xml and update composer.json

dde14b5

dbu reviewed Mar 23, 2015
View reviewed changes

dantleech reviewed Mar 23, 2015
View reviewed changes

Controller/BaseController.php

/**

* Base Controller

*

* @author Daniel Leech <[email protected]>

Copy link

Member

dantleech Mar 23, 2015

Choose a reason for hiding this comment

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

I didn't write this :)

dbu closed this Mar 25, 2015

lsmith77 removed the in progress label Mar 25, 2015

dbu mentioned this pull request Mar 25, 2015

Version upgrades #59

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Version upgrades and cleanups #58

Version upgrades and cleanups #58

briancappello commented Dec 4, 2014

briancappello Dec 4, 2014

lsmith77 commented Dec 4, 2014

briancappello Dec 4, 2014

briancappello Dec 11, 2014

dantleech Dec 12, 2014

dantleech Dec 12, 2014

dantleech Dec 12, 2014

dantleech commented Dec 12, 2014

dbu commented Dec 12, 2014

dantleech commented Dec 13, 2014

briancappello commented Dec 22, 2014

dantleech Dec 22, 2014

dantleech commented Dec 22, 2014

dbu Dec 26, 2014

dantleech Dec 26, 2014

briancappello Dec 26, 2014

lsmith77 Jan 21, 2015

lsmith77 commented Dec 26, 2014

dantleech Dec 26, 2014

briancappello Dec 26, 2014

lsmith77 Jan 21, 2015

dbu Jan 21, 2015

lsmith77 commented Feb 6, 2015

jrmyio commented Mar 22, 2015

wouterj commented Mar 22, 2015

dbu Mar 23, 2015

dbu commented Mar 23, 2015

dantleech Mar 23, 2015

dbu commented Mar 25, 2015


		return $this->renderResponse($contentTemplate, compact('post'));
		}

Version upgrades and cleanups #58

Version upgrades and cleanups #58

Conversation

briancappello commented Dec 4, 2014

General Changes

Persistence

Front End

New Features

Known Issues

Choose a reason for hiding this comment

lsmith77 commented Dec 4, 2014

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 Dec 12, 2014

dbu commented Dec 12, 2014

dantleech commented Dec 13, 2014

briancappello commented Dec 22, 2014

Choose a reason for hiding this comment

dantleech commented Dec 22, 2014

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

lsmith77 commented Dec 26, 2014

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

lsmith77 commented Feb 6, 2015

jrmyio commented Mar 22, 2015

wouterj commented Mar 22, 2015

Choose a reason for hiding this comment

dbu commented Mar 23, 2015

Choose a reason for hiding this comment

dbu commented Mar 25, 2015