Fix #88 and #93

[damienflament]

I implemented the missing tests to reveal the issue #88.

I started by creating a new test method. But after using a data provider, I covered all the existing tests with a single method and the data provider.

The issue #88 is not fixed but I want to show you my work as the test code has been heavily modified. You can follow the modification steps by individually watching the commits.

[damienflament]

I need your opinion on the way we expect the correct behavior of the tested object.

Actually, we expect some setters to be called on the context objects. The point is that the adapter can call this setter to set the expected value, then call it again and set a different value. The resulting context will not be in the expected state, but the tests don't see it !

This issue can be fixed by using immutable URIContext objects as you recommended in your review.

But for now, we can use standard PHPUnit expectation methods on the values returned by the URIContext stub getters.

[dbu]

can we consider the URIContext as a value object? as long as there is no additional logic in it complicating things, i would simply not mock the context object but use a real one, and then make the assertions on what the object represents after the adapter ran through. isolation of unit tests is a good thing, but imho with value objects we don't need to worry too much. too much mocking is also an issue: we want to test the outcome of the adapter, not ensure how the code operates line for line - if we refactor the adapter and it has the same outcome, the tests ideally keep succeeding.

[damienflament]

I'm facing some difficulties while using the URIContext objects due to phpspec/prophecy#335.

[dbu]

would it help to not prophesize the URIContext but use a real instance? or is that what you are triyng to do?

[damienflament]

It's what I'm trying to do. As the behavior of the stubs depends on the UriContext object, I need to configure them for each UriContext instance.

The bug in Prophecy prevents this as it registers only one promise because it tries to group the promises by arguments matches.

[dbu]

now the error looks different. if this is what it takes, i am fine with a test like this using the real collection and context. as long as we have some clean unit test for the collection and the context, it should be ok to rely on them.

[damienflament]

The errors are more readable in my opinion.

as long as we have some clean unit test for the collection and the context, it should be ok to rely on them

Do you want that I improve the context and collection unit tests (if needed) in this PR ? Or in another one, later ?

[dbu]

as long as we have some clean unit test for the collection and the
context, it should be ok to rely on them

Do you want that I improve the context and collection unit tests (if needed) in this PR ? Or in another one, later ?

if you have time to work on it, would be great. but a separate pull request is better, as the two things are not related. the smaller a PR the easier to review and get merged.

[damienflament]

While trying to fix the issue, I need some explanation about the expected behavior of the manager and the adapter.

Currently, the AutoRouteManager replaces the conflicting route with the existing one if they both reference the same content. But it gives the untranslated subject to the adapter.

The point is that two routes can reference the same content in different languages.

I think this issue has not been discovered yet because this check is done only if the manager needs to resolve a conflict. And handling an URI conflict between two routes referencing the same content for different locales is the subject of this PR.

So, what's the expected behavior ?

1) Two contents are the same regardless of the locale:

• it's seems inappropriate to me as, even if PHPCR-ODM can handle translations within an unique document (using the attribute translation strategy), the persisted content remains different,
• an additional check needs to be done by the manager to check if the locales are differents 2) Two contents in different languages are different:
• the manager needs to gives the translated subject instead of the non translated one to the adapter when checking for content equality

[dantleech]

So, what's the expected behavior ?

I think in hindsight passing the UriContext object to compareAutoRouteContent would have been more flexible - but to do this would be a BC break.

WIthout getting deeper into this I am not sure what side-effects passing the translatedSubject will have, I guess passing the UriContext as a third argument might be an acceptable compromise, you could then compare the locales with sthng like:

$this->documentManager->getUnitOfWork()->getLocale($existingRoute) == $uriContext->getLocale()

/cc @dbu

[damienflament]

but to do this would be a BC break.

I think we should not worry about breaking BC. While trying to fix the issue, I need to add parameters to some methods in the ConflictResolverInterface class and their implementations.

That's why I still think that my first solution (make the adapter persist the created route) was the best one. It will break the behavior of the Routing Auto Bundle. But as this bundle is also maintained by you, I think it's less problematic than breaking the BC on interfaces which might be implemented by third parties (the conflict resolver).

[damienflament]

I am not sure what side-effects passing the translatedSubject will have

The translated subject will be compared with the subject contained in the route. Nothing more.

But to make this work, we need to put the translated subject instead of the non translated one when creating the route.

In fact, the best solution might be to pass the URIContext instead of the subject (as you suggested). Then, it's up to the adapter to decide if two contents are equals regardless of the locale. Moreover, it will fix #40.

you could then compare the locales with sthng like: $this->documentManager->getUnitOfWork()->getLocale($existingRoute) == $uriContext->getLocale()

If compareAutoRouteContent should not take care of the locale, I thought I would add a new method compareAutoRouteLocale to the adapter. It's implementation might be:

public function compareAutoRouteLocale(AutoRouteInterface $autoRoute, $locale)
{
    if ($autoRoute->getLocale() === $locale) {
        return true;
    }
    return false;
}

[dbu]

regarding BC: this should be fixed against the upcoming routing-auto 2.0. we tagged a 2.0.0-RC1. while thats a bit late for BC breaks, i don't think there are all that many users implementing the interfaces. so all in all, i am ok with adjusting the interfaces and go for the cleanest solution at this point, rather than introduce a workaround for BC reasons before 2.0.0 is even stable.

[dantleech]

That's why I still think that my first solution (make the adapter persist the created route)

I don't see how this would solve the problem acceptably? It would, if I understand (and I may not), mean the adapter would need to flush the storage in order that the route be persisted and subsequently found (and the conflicts be resolved).

[damienflament]

It would [...] mean the adapter would need to flush the storage in order that the route be persisted

I thought it is possible to search within persisted but not flushed documents. I was wrong because I misunderstood the UnitOfWork pattern.

So, using the UriContextCollection to search for previously created routes is the solution.

But I don't like the fact that the ConflictResolverInterface::resolveConflict method needs the collection as an additional argument to do its job which also requires that the collection object is passed to the URIGenerator.

As the adapter is already injected in the AutoIncrementConflictResolver, could we consider that the adapter have to take care of when the object needs to be really persisted (flushed) and that it has to find previously created routes (regardless their persistence state) ? This could be implemented by searching in the UnitOfWork, but I know its hacky. Keeping a list of created but not flushed routes within the adapter might be the solution.

What do you think about that ? Maybe a better idea ?

[damienflament]

I'm sorry if I'm too insistent but I need to a answer to my previous question:

Actually, the manager behavior is to replace the conflicting route with the existing one if they both reference the same content.

The point is that two routes created for the same UriContextCollection inevitably reference the same content as a UriContextCollection is built for a unique subject. So, the current behavior is to merge all conflicting routes within the collection.

But this behavior does not take care of the locale of the content. In my opinion, two subjects in two different locales are two different content.

[damienflament]

It would [...] mean the adapter would need to flush the storage in order that the route be persisted

I thought it is possible to search within persisted but not flushed documents. I was wrong because I misunderstood the UnitOfWork pattern.

After some testing, it seems I wasn't wrong the first time.

This test pass:

public function testFoo()
{
    $document = new Document('/document');

    $this->dm->persist($document);

    unset($document);

    $this->assertNotNull($this->dm->find(Document::class, '/document'));
}

This can be explained by the fact that the DocumentManager first search within the UnitOfWork which search within its identityMap.

[dantleech]

This test pass:

With this adapter - but this wouldnot work with a hypothetical Doctrine ORM adapter (unless the URI is the identifier, and if the URI is the identifier then you have problems as it is mutable).

In my opinion, two subjects in two different locales are two different content.

They are two views on the same content, those views can have different URIs.

[damienflament]

This test pass:

With this adapter - but this would not work with a hypothetical Doctrine ORM adapter (unless the URI is the identifier, and if the URI is the identifier then you have problems as it is mutable).

Can we consider the adapter responsible of keeping a list of created routes which needs to be flushed ? This way, the current PHPCR-ODM adapter should works without modifications. I like the idea that this component does not have to worry about the persistence, as it is delegated to the adapter.

In my opinion, two subjects in two different locales are two different content.

They are two views on the same content, those views can have different URIs.

They should ! Some people will say "they must". Anyway. I will keep the current compareAutoRouteContent behavior untouched within the tests and add a compareAutoRouteLocale method to the adapter.

Thanks for your answers. I'm sorry about my numerous questions about this PR. But I'm learning about PHPCR, Doctrine and some Symfony components internal functioning at the same time.

[dantleech]

On 30 May 2017 22:43:59 BST, Damien Flament notifications@github.com wrote:

This test pass:

With this adapter - but this would not work with a hypothetical Doctrine ORM adapter (unless the URI is the identifier, and if the URI is the identifier then you have problems as it is mutable).

Can we consider the adapter responsible of keeping a list of created routes which needs to be flushed ? This way, the current PHPCR-ODM adapter should works without modifications. I like the idea that this component does not have to worry about the persistence, as it is delegated to the adapter.

In my opinion, two subjects in two different locales are two different content.

They are two views on the same content, those views can have different URIs.

They should ! Some people will say "they must".

Thinking about it, language could also be determined by sub domain, browser, session etc. Worth considering.

Anyway. I will keep the current compareAutoRouteContent behavior untouched within the tests and add a compareAutoRouteLocale method to the adapter.

Thanks for your answers. I'm sorry about my numerous questions about this PR. But I'm learning about PHPCR, Doctrine and some Symfony components internal functioning at the same time.

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

[dbu]

@dantleech can you review these changes again? i would still be ok to sneak this into 2.0.0 but we would need to wrap this up soonish.

@damienflament can you edit the CHANGELOG.md and add notifications for the BC breaks please?

[damienflament]

As mentioned in my comment to #93, merging this PR right now will break the conflict resolving. That's why it's still marked as WIP.

Should I fix #93 in this PR ? Or in another one. Then you will merge them at the same time.

@dantleech can you review these changes again?

Your review is welcomed !

I'm not really satisfied with the test code structure.

Moreover, I don't like the fact that the UriGeneratorInterface::resolveConflict method needs the UriContextCollection as an additional argument. It will be the same for the ConflictResolverInterface::resolveConflict method.

Any suggestion ?

[dantleech]

Would it make sense to make the UriContextCollection a part of the UriContext object? It makes some sense to me that the other URIs form part of the context here - and it can be injected with the UriContextCollection::createContext factory method - which is the only place (outside of tests) that this object is created (the tests would need to be adapted). This would mean no BC break (?).

1. What do you think?

Otherwise it seems to me that we shouldn't let the adapter have the responsiblity for checking the UriContextCollection for the existing route (it should have the explict responsbility of checking the storage which it represents). But if major refactoring is required, then let's not worry about it.

2. Is there a way to push back the check?

And thanks alot for your work here, I think it's a great addition :)

3. We should test this branch with the functional tests in the bundle (this is typically where I would expect to find regressions).

[damienflament]

Would it make sense to make the UriContextCollection a part of the UriContext object

It totally makes sense ! I don't like cyclic dependencies. But here a context is closely related to the collection, as a collection is related to a single subject and the context is also related to this subject. This relation might be more evident if the UriContext::getSubject method returns the subject of its collection instead of storing it in a property.

Moreover, as you said, the API of the UriContext class will be modified, but there will not be any BC break, I think.

Otherwise it seems to me that we shouldn't let the adapter have the responsiblity for checking the UriContextCollection for the existing route

Seems evident for me.

We should test this branch with the functional tests in the bundle

This check will be the next step after you agree with this PR. I saw the test suite of the bundle is already configured in this component to be run if found. I don't try it; but it should not be too difficult.

[damienflament]

Seems OK for me.

The AutoIncrementConflictResolver tests have been updated to take care of the existing routes within the UriContextCollection.

I ran the bundle tests using the conflict-resolving branch of the damienflament/routing-auto-bundle repository. The only BC break is the new AdapterInterface::compareAutoRouteLocale method. It is implemented in the bundle by symfony-cmf/routing-auto-bundle#202.

Feel free to review the last changes. Thanks !

[dantleech]

Yes, I will try and look at this properly in the next few days :)

On Sun, Jun 11, 2017 at 11:43:13PM -0700, David Buchmann wrote:

@dbu commented on this pull request.

thanks a lot for the work, and all the doc cleanups! i found two nitpicks i'd like to improve.

[1]@dantleech can you please verify if this seems ok to you or if you see any issues?

---

In [2]src/AutoRouteManager.php:

@@ -124,18 +124,41 @@ public function handleDefunctRoutes() }

  /**

• • Find an existing route which matches the URI of the given context.

• *

• • It is searched within the currently processed collection and the already
• • persisted routes (using the adapter).

• */

• private function findExistingRoute(UriContext $uriContext)

• {

• $uri = $uriContext->getUri();

• $existingRoute = null;

  this line is unneccessary, as the variable is overwritten on the next code line below

  ---

  In [3]src/ConflictResolver/AutoIncrementConflictResolver.php:

           $candidateUri = $this->incrementUri($uri);

  }

    return $candidateUri;

  }

• /**

• • Tell if the given URI for the given context is conflicting with another
• • route.

• */

• protected function isUriConflicting($uri, UriContext $uriContext)

• {

• if (null === $uriContext->getCollection()->getAutoRouteByUri($uri)

• && null === $this->adapter->findRouteForUri($uri, $uriContext)) {

• return false;

  please simplify this method to return null !== $uriContext->getCollection()->getAutoRouteByUri($uri) || null !== ...findRouteForUri...;

  — You are receiving this because you were mentioned. Reply to this email directly, [4]view it on GitHub, or [5]mute the thread.

  Reverse link: [6]unknown

References

Visible links

1. https://github.com/dantleech
2. https://github.com/symfony-cmf/routing-auto/pull/91#discussion_r121318284
3. https://github.com/symfony-cmf/routing-auto/pull/91#discussion_r121318756
4. https://github.com/symfony-cmf/routing-auto/pull/91#pullrequestreview-43355651
5. https://github.com/notifications/unsubscribe-auth/AAgZcYstT-CnY0vm92F71mattB_EK18Tks5sDN4BgaJpZM4NgRoR
6. https://github.com/symfony-cmf/routing-auto/pull/91#pullrequestreview-43355651