Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Route conflicts #187

Open
oliverklee opened this issue Oct 5, 2017 · 4 comments
Open

Route conflicts #187

oliverklee opened this issue Oct 5, 2017 · 4 comments
Assignees

Comments

@oliverklee
Copy link
Contributor

@samtuke, I've done some testing and some research on representing conflicts for phpList modules that provide the same routes (e.g., "/" or "/api/v2/"):

  • When a route with a path already is set, it cannot be overwritten - the first route "wins", and the second route with the same path is silently ignored.
  • The installation order of Composer packages is that requirements are installed first. So if another module depends e.g., on the phplist/web-frontend package (and our package provides the "/" route), the route from phplist/web-frontend will win, i.e., the "official" route will win. If there is no dependency, the order is not fixed, though.
  • As far as I can see, there is no way to have something like "this package provides the startpage-route capability, and it conflicts with all packages that also provide this route" in a composer.json, as in that case the package would be in conflict with itself.

I've come to the conclusion that the risk of conflicting routes is relatively low anyway, and by properly using the dependencies, the official route "wins". So I'd like to not add any additional conflict mechanism, if there are no objections. @samtuke, @michield, any opinions on this?

@oliverklee oliverklee added this to the 4.0.0 ("phase 2") milestone Oct 5, 2017
@oliverklee oliverklee self-assigned this Oct 5, 2017
@samtuke
Copy link
Collaborator

samtuke commented Oct 6, 2017

@oliverklee That's enlightening, thanks.
It's good that the required official packages will 'always win', but this could be confusing for plugin developers when their routes are silently overwritten. Is there no way to throw an exception or otherwise provide feedback to developers if their module routes are already registered? I agree this is an unlikely scenario, at least initially.
Perhaps we could add as a todo a feature which logs errors in such cases, clearly document the risk in the plugin docs, and implement the solution after more core functionality of phpList4-core and the REST API are implemented?

@oliverklee
Copy link
Contributor Author

I've conducted more research and done some testing.

Symfony itself does not "notice" if there are two routes with the same URL (or two different patterns that will resolve to the same URL, or two patterns that will match the same URLs in some cases). It just goes through all routes and takes the first once that matches the current request URL.

So, in theory, we could write something like a route linter that goes through all routes and at least warns the patterns are exactly the same. To test whether two different patterns are equivalent, we would need to construct once deterministic finite automaton for each patterns and then test those for equivalence. This is possible, but PSPACE-complete, i.e., quite a lot of work (see https://cs.stackexchange.com/questions/12267/algorithm-to-determine-whether-two-regexes-are-equivalent and https://en.wikipedia.org/wiki/PSPACE-complete).

So doing any automatic checking is not feasible.

What we could do is that we require the prefix option being set in each route configuration in the composer.json and then throw an error if two routes have the same prefix. This will not prevent prefixes that are prefixes of other prefixes (e.g., "api/v2/" and "/" as for the REST API and the web frontend (planned)), though.

@samtuke What do you think of this?

@samtuke
Copy link
Collaborator

samtuke commented Oct 10, 2017

@oliverklee It sounds like the composer.json / prefix approach is the best or only technical option currently identified, and yet it would not protect the most important routes from being overridden.
Let's make a mental and documentation note about the threat of clashing routes and put the issue to one side for revisitation in future (when third party modules exist and more benefit to be gained from resolving it).

@oliverklee oliverklee mentioned this issue Oct 11, 2017
17 tasks
@oliverklee oliverklee modified the milestones: 4.0.0 ("phase 2"), phase 3 Oct 11, 2017
@oliverklee
Copy link
Contributor Author

Agreed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants