[WIP] Branch for refactoring to enable actions on content changes.

[dantleech]

See #34

[dantleech]

@wouterj this is what I roughly have in mind for the configuration format - but it could all change as I need to get back into the code and understand what it wants to do.

[dantleech]

There is also a question over if this should even be in the dependency injection configuration.

In 1.1 I think we should move away from DI configuration for mapping to documents and handle things more like the ORM and the framework extra bundle etc. I.e. native support for annotations, xml and yaml in seperate configuration files.

[dantleech]

I wanted to do

on_content_change:
    - [ leave_redirect, { foo: bar } ]

But the configuration class was driving me crazy -- is this possible?

[wouterj]

Yes it's possible when you use beforeNormalization. Not sure I like it though. We dhould keep it as a shortcut. I also think we should support both an array of strategies and a single strategy. I'll have some time tomorrow to update this PR for this, ok?

[dantleech]

So how would supporting both work? Would a single look like the following? :

on_content_change:
    strategy: foobar
    options: { }

If you agree with the format in this PR for "on_content_change" I prefer to not do any more work on Configuration in this PR - so it can be focussed on the content_change event. There is #38 which has a proposition for a new configuration format - perhaps we can work on that?

[dantleech]

Reduced the scope of this PR so that it doesn't break the configuration, see #38 for the original PR.

[dantleech]

Have made some significant progress with this.

There is another PHPCR-ODM problem however.

The LeaveRedirect content changed strategy wants to insert a new document at the path of the original AutoRoute - however the order in which the UnitOfWork executes the updates prevents this, and I recieve a "node at "foobar" already exists":

            $this->executeInserts($this->scheduledInserts);
            $this->executeUpdates($this->scheduledUpdates);
            $this->executeRemovals($this->scheduledRemovals);
            $this->executeReorders($this->scheduledReorders);
            $this->executeMoves($this->scheduledMoves);
            $this->session->save();

I.e. I can make this work by moving "executeMoves" to the top.

I remember this issue cropping up before -- can we refactor the UOW to execute the operations in the order they were made?

[dbu]

i think that is really hard, as some operations are determined implicitly (e.g. when we see that a child is no longer present in a collection). but indeed, for direct calls to persist with new document, remove and move, we should be able to build some sort of log and flush that. we had to do this for jackalope, see jackalope/jackalope#140 . the alternative would probably be to record what we will have to do and then listen to postFlush and do our things and follow with another flush.

btw, @nicolas-bastien i talked with you about that topic at forumphp, right?

[nicolas-bastien]

Hi,

I'm not sure to get all the conversation in detail but what I discuss with @dbu is to automaticaly create a RedirectRoute when a route is moved, so that previous url is used somewhere on the web it will still be working.

If I understand here it's just for AutoRoute, don't you think we should to that for Route directly ?

[dbu]

@nicolas-bastien actually i think what you do is also sort of autorouting (@dantleech nicolas uses a text field that you can use to customize the generated url).

but we might want to have a configuration saying for what class / interface the redirections should be created. then you could either just configure that to the standard route document, or create your own service for that. i assume the AutoRoute needs no specific methods for the listener to work, right?

[dantleech]

@dbu what would be the problem with having a single operations queue $scheduledOperations = array()?

[dantleech]

@nicolas-bastien for your issue, I created an issue: #41

[dbu]

operations queue: i guess it could work, yes. for automatically determined operations like a child is no longer in a children collection, they would just be appended to that queue as they are encountered during flush, but that should be ok. only thing i don't know is how it will work out with event handling during the flush - deletes may be encountered at any point, now what if a listener creates another operation? this sounds like a rather heavy change.
i agree it makes a lot of sense to do (here again, phpcr is not the same as the orm). how do we proceed? the cmf has code freeze this weekend - i am unsure if we can rush that in in time. i am happy to review the PR but won't have time to put much work into anything before mid december. could we bring the RoutingAutoBundle to a stable version once the non-flushed multilang problems are sorted out, and postpone adding redirect handling to the next version?

btw, be aware that move and remove are cascaded automatically and without any notification to their children (its phpcr itself updating the children paths as they move with their parent, or deleting all children of a deleted node) - not sure if that matters for you.

[dbu]

oh, or could we simply go with a postFlush approach and do a follow-up flush until phpcr-odm is refactored?

[dantleech]

The post flush approach would involve refactoring the RoutingAutoBundle a little bit - I guess we could do that - what are the disadvantages of that approach other than it being ugly?

[dbu]

performance, but i guess write is rather slowish anyways, and much less frequent than read so i could live with that. compared to having to manually create the route, its still a LOT faster (milliseconds rather than a support call for the user :-)

if we manage to do it in postFlush, we can then still later opt for the better approach once phpcr-odm is improved. the doctrine listener would simple record the things it needs to know to do the things it wants to do on the post flush. (maybe store strings rather than documents, to avoid troubles with meanwhile changing state)

[dantleech]

I've done a POC for an operation queue implementation here:

https://github.com/dantleech/phpcr-odm/pull/4/files

It destroys the tests...

[dantleech]

@dbu I think I would rather fix PHPCR-ODM than add the 2 flush hack. As noted above I had a go at a 100% operation stack approach - not sure how practicable that is though.

[dbu]

i think the operation queue approach is not ready to go into 1.1 and we should not delay for too long. for 1.2 its totally an option. i think it would be great to first get this working with the 2 flushes and later refactor once it becomes possible.

[wouterj]

@dantleech fixed the configuration. It contains some duplicated code, but if we want to work around that, we'll make the code very dirty...

[dantleech]

Thanks wouter! I guess we can always clean it up later if it becomes possible.

[dantleech]

This is "working" now except for the problem with committing extra documents in the onFlush event.

[dantleech]

ok. Moved the operation to postFlush and it "works" but depends on bugfix in jackalope-doctrine-dbal: https://github.com/jackalope/jackalope-doctrine-dbal/pull/163/files

[dantleech]

Wlll be fixed in #72