[WIP] Support for handling defunct routes #81

dantleech · 2014-03-25T21:32:18Z

No description provided.

- How to correctly transfer the children of a PHPCR-ODM defunct auto route to the new route?

dantleech · 2014-03-28T21:11:45Z

quite alot of refacvtoring. Renamed OperationStack to UrlContextStack.

The defunct route handler stuff is now plumbed into the ServierRegistry and DI compiler pass.

We need to change from URL to URI.

WIP.

dantleech · 2014-03-30T08:27:50Z

The remove defunct route functionality works now.

I have introduced phpspec ...

dantleech · 2014-03-30T08:30:00Z

AutoRoute/DefunctRouteHandler/DelegatingDefunctRouteHandler.php

+ *
+ * @author Daniel Leech <[email protected]>
+ */
+class DelegatingDefunctRouteHandler implements DefunctRouteHandlerInterface


So the AutoRouteManager accepts one DefunctRouteHandler, this one will use delegate to handlers registered with the ServiceRegistry

dantleech · 2014-03-30T08:59:52Z

Tests/Spec/Symfony/Cmf/Bundle/RoutingAutoBundle/AutoRoute/UrlContextStackSpec.php

+use Symfony\Cmf\Bundle\RoutingAutoBundle\AutoRoute\UrlContext;
+use Symfony\Cmf\Bundle\RoutingAutoBundle\Model\AutoRouteInterface;
+
+class UrlContextStackSpec extends ObjectBehavior


First spec .. wdyt?

+1 (although we should change all PHPunit tests to PHPspec)

I am ok with that in theory -- but is what would be the best way to do:

public function providesFoobar() { return array( array('...', '...';, '...'), array('...', '...';, '...'), ); } /** * @dataProvider provideFoobar */ public function foobar($a, $b, $c) { }

Then again, I don;t really care if we have the two at the same time. At least for the time being.

http://stackoverflow.com/a/19476214/1149495

Thought that might be the case. That kinda sucks.

@dev

- Fixed spec method names - Removed @dev from phpspec dep. - Added phpspec script to travis

- Applied php-cs-fixer - Fixed refresh command (well, the test passes)

dantleech · 2014-03-31T21:18:26Z

ok this is getting there.

todo:

Fix all of the unit tests (or migrate them to phpsepc)
Add a RedirectRoute defunct route handler
Merge into mapping and add the missing mappings
Complete the functional testing.

dantleech · 2014-04-03T19:51:25Z

I have run into a wall with PHPCR-ODM.

At the moment I am removing routes in the ODM listener in the post-flush event. This works, but if I want to leave a redirect route this becomes awkward, and it is the wrong way to do things - and other implementations would not do things in this order.

The best way to handle all of this is during the flush -- but PHPCR-ODM does not support deleting a route and then creating a new route in its place (i.e. replacing a route).because of the sequence of operations: doctrine/phpcr-odm#396

There is also the issue with getReferrers not returning non-persisted documents: doctrine/phpcr-odm#464

At this point I could bypass the ODM completely and create the objects with the ODM mappings directly with PHPCR - or stop doing RoutingAuto and invest the time instead into PHPCR-ODM. Hm. Seems a shame as this PR is so close to being finished.

// cc @dbu @lsmith77

lsmith77 · 2014-04-03T19:53:16Z

Yeah it is very tricky .. we tried to do a replace for the ODM and always got stuck some where and ended up saying for now it just requires a 2 step flush.

dbu · 2014-04-04T10:45:15Z

i would really do it in two steps for now. that way we have the feature, and if its used a lot and the double flushes are really a performance issue, we can go and try to improve the odm (i guess it always goes down to the operations queue). for the referrers, i just commented on 464 - i see no way we could provide that reliably but performant.

dantleech · 2014-04-04T11:18:20Z

I could somehow support firing a "handle defunct routes" event before and after the flush, enabling different implementations (adapters) to handle things at different times.

Handling it 100% in PHPCR still holds some appeal, as I think it could be done safely before the flush - the only problem would be converting the Document to a Node without duplicating too much logic from the UOW.

dbu · 2014-04-04T11:23:24Z

i think events would be a nice thing here. i can imagine people need
their own logic for redirects.

the phpcr handling will not solve the referrers issue. but for changing
the document class, would
doctrine/phpcr-odm#420 not be enough for what
you need?

dantleech · 2014-04-04T11:28:48Z

The PHPCR thing would solve the referrers issue, as we would save after creating the route (assuming the x Session doesn't support non-persisted referrers).

Migration might be an option, but it really doesn't make any difference if we do a two step thing here, as we would delete in the flush, and create in the post flush, avoiding the order of operations problem.

dbu · 2014-04-04T11:36:23Z

but if you need to flush, why not then use the dm then? operating on phpcr directly will make it much harder if somebody works with custom documents.

dantleech · 2014-04-04T12:14:50Z

If I save the session with PHPCR I only persist the changes for the Nodes that I have modified (because the DM will only persist to the PHPCR session when executing the flush, so the Phpcr "uow" is empty). But if I flush the DM prematurely that could cause problems and edge cases.

Hmm, for the custom documents we would need to map the document to PHPCR, in which case it would be nice to refactor the UOW to use a document => node mapper of some sort that I could steal.

But basically I would be encapsulating a big hack.

Will try and implement the extra flush first anyway.

dantleech · 2014-04-06T06:01:28Z

AutoRoute/Mapping/Loader/XmlFileLoader.php

+        $defunctRouteHandlerLength = $defunctRouteHandlerNodes->length;
+
+        if (1 < $defunctRouteHandlerLength) {
+            throw new \InvalidArgumentException(sprintf('There can only be one defunct route handler per mapping, %d given for "%s" in ""%s', $defunctRouteHandlerLength, $className, $path));


@wouterj why do we check this here when the loader validates the XML with a schema?

hmm, this is indeed not needed. However, the current XML schema defines "unbounded" as maxOccurs for the <defunct-route-handler> element. This should be updated to 1 if we don't support more than 1 per mapping.

dantleech · 2014-04-06T06:28:37Z

AutoRoute/DefunctRouteHandler/DelegatingDefunctRouteHandler.php

+        $metadata = $this->metadataFactory->getMetadataForClass($realClassName);
+
+        list($name, $options) = $metadata->getDefunctRouteHandler();
+        $defunctHandler = $this->serviceRegistry->getDefunctRouteHandler($name);


@wouterj this is always array('remove', array()) when we have configured: array('leave_direct', array()). The class metadata is populated with leave_redirect, but for some reason it gets reverted.

Fixed with https://github.com/symfony-cmf/RoutingAutoBundle/pull/81/files#diff-c63077db509b72afbe3343b6cf9576f8R89

dantleech · 2014-04-06T06:31:19Z

Updated, added defunct_route_handler to XML and YAML mappings and added functional test for LeaveRedirectRoute.

Am planning to go full PHPCR for leaving the redirect routes -- I don't see many disadvantages tbh, the redirect route class can still be configured by the user. The order of operations in PHPCR-ODM simply makes any attempt to do this there too hacky.

dantleech · 2014-04-10T21:12:08Z

ok getting there. the next push should see the LeaveReditrectHandler in a working state then we can merge this and look to getting something in master.

But needs an additional flush

dantleech · 2014-04-16T06:46:35Z

LeaveRedirect now works, but it requires an additional Flush. I am not really too keen on using the postFlush event to call flush, as postFlush is actually called within flush, after the commit but before the flush has finished: https://github.com/doctrine/phpcr-odm/blob/master/lib/Doctrine/ODM/PHPCR/UnitOfWork.php#L1959

I wonder if there is any scope for adding some sort of re-flush stack? So that listeners can add new flushes easily? Or is the idea generally that re-flushing is bad? (but in this case unavoidable).

/cc @dbu @beberlei

dantleech · 2014-04-16T06:48:28Z

Tests/Functional/EventListener/AutoRouteListenerTest.php

+        $this->getDm()->flush();
+
+        // additional flush -- maybe we should handle this with an event listener of some sort?
+        $this->getDm()->flush();


Additional flush to flush the new Redirect Routes..

dantleech · 2014-04-27T16:26:54Z

@wouterj if its OK with you I want to get this thing finished - the new version has feature parity now I believe - there is still an open issue about how to persist the new RedirectRoutes (doctrine/phpcr-odm#481) but as this is an additional feature, I don't think it should be a blocker.

As for behat and phpspec -- lets just stick with PHPUnit for now - I will revert my phpspec's to phpunit. I really want to get this finished so I can concentrate on other stuff :)

So basically I will try and get all this in master soon-ish and update the documentation and tutorials.

wouterj · 2014-04-27T17:41:45Z

@dantleech feel free to merge this PR when it's finished. I want to do one big fix commit fixing CS etc. after it and then I'm ok with merging it into master. We can still improve things (like behat & phpspec) on the master branch.

dantleech · 2014-04-27T18:24:02Z

hmm .. I am also torn between if we should carry on using PHPUnit or switch to PHPSpec - it doesn't make much sense to have them both, and PHPSpec is alot quicker to work with, whilst PHPUnit is more mature.

Also docblock

dantleech · 2014-05-02T09:41:17Z

@wouterj We should merge this now.

The persisting of new redirect routes currnetly depends upon a non-existing ODM event (trying to get it added to the ORM before adding it to the ODM: doctrine/orm#1022 (comment)). If that doesn't work we can maybe do a temporary hack with the kernel/console terminate events.

Also there is a single failing Mapping test realted to merging, which I wasn't sure about.

So in summary, there should be about 2 failing tests, one mapping and one for redirects.

I am only temporarily online now, could you merge if it looks ok?

[WIP] Support for handling defunct routes

Support for handling defunct routes

6181285

dantleech changed the title ~~Support for handling defunct routes~~ [WIP] Support for handling defunct routes Mar 25, 2014

dantleech added 2 commits March 28, 2014 20:56

Refactored OperationStack to UrlContextStack

2c1860d

Refactoring, probably on the last problem

c0561f6

- How to correctly transfer the children of a PHPCR-ODM defunct auto route to the new route?

Remove auto route OK

cabd363

dantleech reviewed Mar 30, 2014
View reviewed changes

Fixed defaults persistance

4f5d66b

dantleech reviewed Mar 30, 2014
View reviewed changes

dantleech added 4 commits March 31, 2014 22:29

Refactored route => autoRoute

36395e1

Various

006a4b9

- Fixed spec method names - Removed @dev from phpspec dep. - Added phpspec script to travis

Removed is_initializable method

7c4779e

Various

5ebd041

- Applied php-cs-fixer - Fixed refresh command (well, the test passes)

Added support for defunct route handler to mapping

6dfae25

dantleech reviewed Apr 6, 2014
View reviewed changes

Adding defunct handler functional test

fa8b85a

dantleech reviewed Apr 6, 2014
View reviewed changes

Fixed metadata problem

388062e

LeaveRedirect "works"

2cef3cd

But needs an additional flush

dantleech reviewed Apr 16, 2014
View reviewed changes

dantleech added 7 commits April 30, 2014 18:05

Fixed OdmAdapter unit tests

d3ad7fc

AutoRouteInterface implements RouteObjectInterface

3b87d96

Also docblock

Fixed AutoRouteManagerTest

f3bffd6

Updated core bundle dep

5041179

Fixed token provider tests

f401226

Fixed UrlGenerator test

62c4f53

Fixed final tests

26c9b5a

dantleech added a commit that referenced this pull request Jun 10, 2014

Merge pull request #81 from dantleech/defunct_route_handling

5b4477d

[WIP] Support for handling defunct routes

dantleech merged commit 5b4477d into symfony-cmf:mapping_refactor Jun 10, 2014

dantleech deleted the defunct_route_handling branch June 10, 2014 19:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Support for handling defunct routes #81

[WIP] Support for handling defunct routes #81

dantleech commented Mar 25, 2014

dantleech commented Mar 28, 2014

dantleech commented Mar 30, 2014

dantleech Mar 30, 2014

dantleech Mar 30, 2014

wouterj Mar 30, 2014

dantleech Mar 31, 2014

wouterj Mar 31, 2014

dantleech Mar 31, 2014

dantleech commented Mar 31, 2014

dantleech commented Apr 3, 2014

lsmith77 commented Apr 3, 2014

dbu commented Apr 4, 2014

dantleech commented Apr 4, 2014

dbu commented Apr 4, 2014

dantleech commented Apr 4, 2014

dbu commented Apr 4, 2014

dantleech commented Apr 4, 2014

dantleech Apr 6, 2014

wouterj Apr 6, 2014

dantleech Apr 6, 2014

dantleech Apr 10, 2014

dantleech commented Apr 6, 2014

dantleech commented Apr 10, 2014

dantleech commented Apr 16, 2014

dantleech Apr 16, 2014

dantleech commented Apr 27, 2014

wouterj commented Apr 27, 2014

dantleech commented Apr 27, 2014

dantleech commented May 2, 2014

[WIP] Support for handling defunct routes #81

[WIP] Support for handling defunct routes #81

Conversation

dantleech commented Mar 25, 2014

dantleech commented Mar 28, 2014

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

Choose a reason for hiding this comment

dantleech commented Mar 31, 2014

dantleech commented Apr 3, 2014

lsmith77 commented Apr 3, 2014

dbu commented Apr 4, 2014

dantleech commented Apr 4, 2014

dbu commented Apr 4, 2014

dantleech commented Apr 4, 2014

dbu commented Apr 4, 2014

dantleech commented Apr 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

dantleech commented Apr 6, 2014

dantleech commented Apr 10, 2014

dantleech commented Apr 16, 2014

Choose a reason for hiding this comment

dantleech commented Apr 27, 2014

wouterj commented Apr 27, 2014

dantleech commented Apr 27, 2014

dantleech commented May 2, 2014