Massive refactoring #73

dantleech · 2014-03-02T11:52:21Z

OK this is basically a rewrite.

URL schema
URLs now treated as entire URLs, not they're component parts
Droped per token/provider exists/not_exists in favor of conflict resolution on the URL entire.
Abstracted all database logic to driver
Support for handling old routes

FQN\Document\Foobar:
    url_schema: /articles/%article_title%
    conflict_resolver: [ auto-increment ]
    token_providers:
        article_tilte: [ method, { name: getTitle } ]

Note I deleted all the providers just to be Zen - but I will probably pull them back in later.

Now my head really hurts .. but the good news is that the code is much easier to follow.

dantleech · 2014-03-02T11:56:13Z

Doctrine/Phpcr/AutoRouteListener.php


+                foreach ($operationStack->getPersistStack() as $document) {
+                    $dm->persist($document);


The operation stack will also contain a list of documents to remove, potentially. (although for now I am going to make the PHPCR-ODM driver bypass the ODM because its buggy).

dantleech · 2014-03-02T16:25:06Z

btw, would any body object to using Behat to replace the functional tests? /cc @wouterj @dbu @lsmith77

dantleech · 2014-03-02T17:55:07Z

Added unit test for the AutoRouteManager. Am going to sign off on this until Tuesday evening.

wouterj · 2014-03-02T18:20:08Z

would any body object to using Behat to replace the functional tests

I really love Behat, so no. Except that we choose to stick with PHPunit only, so a new contributor only has to learn knowledge about PHPunit and not about other tools as well. If we step of that, I also prefer to use PHPspec for unit tests, or use another mocking framework like Mockery or Prophecy.

Am going to sign off on this until Tuesday evening.

Ok, thank you for starting on with this. I'll see if I can take it somewhat further :)

wouterj · 2014-03-02T22:29:24Z

Tests/Unit/AutoRoute/Mapping/Loader/YmlFileLoaderTest.php

@@ -1,139 +0,0 @@
-<?php


Should be readded

dantleech · 2014-03-08T21:16:10Z

Added a unit test for the UrlGenerator using Prohpecy just as an experiment. Much easier than PHPUnit but we will need to create a base test class.

Also rebased on lastest metadata changes.

wouterj · 2014-03-09T12:17:48Z

Tests/Unit/AutoRoute/UrlGeneratorTest.php

+            ->willReturn($tokenProviderConfigs);
+
+        $this->metadata->getUrlSchema()->shouldBeCalled()
+            ->willReturn($urlSchema);


This is a code smell you have learned from PHPunit mocks/mockery, which you shouldn't apply to Prophecy.

You almost always have 2 types of methods: Query and Command. A query method returns a value and a command method contains logic to handle values. Query methods should have promises (e.g. ->willReturn()), while a command method should have predictions (e.g. ->shouldBeCalled()). Only in some rare cases, there is a method which is both a query and a command (this violates CQRS). In that case, you can use both promises and predictions on one method.

In this test, all mocked methods are query methods which should only get promises. We shouldn't test if a getUrlSchema is called, the only thing we need to define is what it returns in case it is called.

makes sense, wasn't 100% about adding those. What would be an example of a Command method? something that does not contribute to the return value but should be called anyway? (like an API call to a web service?)

For instance, $documentManager->persist($doc) is a command method. $context->addViolation(...) is one too.

And a case in this bundle: When testing UrlGenerator#resolveConflict() the ConflictResolver#resolveConflict() method is a command.

- Droped per token/provider exists/not_exists - Abstracted all database logic to driver - URLs now treated as entire URLs, not they're component parts - Support for handling old routes

- Used prohpecy as an experiment - Rebased on the new metadata implementation

- Renamed driver to adapter -

dantleech · 2014-03-13T22:13:41Z

Rebased and added unit test for the PhpcrOdmDriver. I am about ready to start writing functional tests.

@wouterj would you be in favor of introducing behat?

dantleech · 2014-03-13T22:16:04Z

AutoRoute/Adapter/PhpcrOdmAdapter.php

+
+        // bypass the ODM ... but changes will still only be
+        // persisted when the PHPCR session is saved in the ODMs flush().
+        NodeHelper::createPath($this->dm->getPhpcrSession(), $parentPath);


This static call is a pain because I have to mock both a PHPCR SessionInterface and also an associated expectation to return a NodeInterface. @dbu do you think there is any scope for adding an instantiable version of this class in PHPCR utils? would make testing easier :)

yeah, whenever you try to cheat you get caught sooner or later :-(
i was too lazy to pass those helper all around the place. we can't even make the SessionInterface provide this, as its only in phpcr util. if we had built this as a class with methods, we could allow to set the helper and have a getter that creates the instance if its not set. but this would be a major BC break to change that now. we could introduce a new class in parallel but the best name is already taken. and whatever we do, it won't be phpcr-utils 1.1 but 1.2 so i fear right now we do need to mock. but open an issue on phpcr-utils that we should fix this there, and in jackalope and phpcr-odm.

Created: phpcr/phpcr-utils#106

wouterj · 2014-03-14T15:08:00Z

@wouterj would you be in favor of introducing behat?

I like behat, so I have nothing against it. :)

wouterj · 2014-03-14T15:48:34Z

continues in #72

dantleech reviewed Mar 2, 2014
View reviewed changes

wouterj reviewed Mar 2, 2014
View reviewed changes

Tests/Unit/AutoRoute/Mapping/Loader/YmlFileLoaderTest.php

@@ -1,139 +0,0 @@

<?php

Copy link

Member

wouterj Mar 2, 2014

Choose a reason for hiding this comment

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

Should be readded

wouterj mentioned this pull request Mar 3, 2014

Refactor to URL Schema #72

Merged

4 tasks

wouterj reviewed Mar 9, 2014
View reviewed changes

dantleech added 4 commits March 13, 2014 21:42

Massive refactoring

6591bad

- Droped per token/provider exists/not_exists - Abstracted all database logic to driver - URLs now treated as entire URLs, not they're component parts - Support for handling old routes

Added AutoRouteManagerTest

ec71ad6

Added unit test for UrlGenerator

3e5054d

- Used prohpecy as an experiment - Rebased on the new metadata implementation

Added Unit test for PhpcrOdmAdapter and other stuff

3137270

- Renamed driver to adapter -

dantleech reviewed Mar 13, 2014
View reviewed changes

wouterj closed this Mar 14, 2014

wouterj deleted the url_schema_refactor branch April 24, 2015 22:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Massive refactoring #73

Massive refactoring #73

dantleech commented Mar 2, 2014

dantleech Mar 2, 2014

dantleech commented Mar 2, 2014

dantleech commented Mar 2, 2014

wouterj commented Mar 2, 2014

wouterj Mar 2, 2014

dantleech commented Mar 8, 2014

wouterj Mar 9, 2014

dantleech Mar 9, 2014

wouterj Mar 9, 2014

wouterj Mar 9, 2014

dantleech commented Mar 13, 2014

dantleech Mar 13, 2014

dbu Mar 14, 2014

dantleech Mar 16, 2014

wouterj commented Mar 14, 2014

wouterj commented Mar 14, 2014


		foreach ($operationStack->getPersistStack() as $document) {
		$dm->persist($document);

Massive refactoring #73

Massive refactoring #73

Conversation

dantleech commented Mar 2, 2014

Choose a reason for hiding this comment

dantleech commented Mar 2, 2014

dantleech commented Mar 2, 2014

wouterj commented Mar 2, 2014

Choose a reason for hiding this comment

dantleech commented Mar 8, 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

dantleech commented Mar 13, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wouterj commented Mar 14, 2014

wouterj commented Mar 14, 2014