#590 duplicate root route when it in config #992

moshenskyDV · 2018-06-11T19:56:12Z

I am targeting this branch, because its patch.

Closes #590
Code is not my idea, thank @grimpows

Changelog

Updated RoutePageGenerator. Now, homepage creates statically, and it stay as root page for another config pages by default. If we will create in config page with root path ("/") - they same as other will child page (it means it will available by two slashes path ("example.com//").
This patch, before creating homepage looks in config page and search any page with root path. If it will found - we will use that config page as root.
So, after patch, possible configure bundle for homepage in routing file.

Fixed

Creating homepage from router config (check if page available in config - we will not create default Homepage.

Security

I checked this on my project. Looks good. Checked generating new routes and update previously routes.
Please double check this code.

moshenskyDV · 2018-06-11T19:58:47Z

Wait pls, forgot to check lint.

greg0ire · 2018-06-11T20:35:14Z

src/Route/RoutePageGenerator.php

+                    ]);
+                }
+            }
+            $root = ($root) ? $root : $this->pageManager->create([


Why the parenthesis? Also $root ?: $this->pageManager->create([… should be equivalent, right?

greg0ire · 2018-06-11T20:35:30Z

Please write a changelog directed at end users. Keep in mind that they probably do not care what file or class you changed, but what features you provided / what bugs you fixed. Try to answer the question "What benefit do I get from upgrading?"
More on http://keepachangelog.com

core23 · 2018-06-12T04:18:18Z

src/Route/RoutePageGenerator.php

@@ -89,7 +89,22 @@ public function update(SiteInterface $site, OutputInterface $output = null, $cle

        // no root url for the given website, create one
        if (!$root) {
-            $root = $this->pageManager->create([
+            // first search for a route
+            foreach ($this->router->getRouteCollection()->all() as $name => $route) {


Can you please create a separate method for this.

moshenskyDV · 2018-06-12T17:48:05Z

Travis said, that lint issues in another files (tests). Should i fix this, or not?

greg0ire · 2018-06-12T20:21:35Z

src/Route/RoutePageGenerator.php

+     * If root path available in config - create from config
+     * Else - generate it statically.
+     *
+     * @param SiteInterface $site


Please remove this phpdoc

greg0ire · 2018-06-12T20:23:49Z

src/Route/RoutePageGenerator.php

+            if ('/' === $route->getPath()) {
+                $requirements = $route->getRequirements();
+                $name = trim($name);
+                $root = $this->pageManager->create([


Why not return here directly? This would make the rest of the code less complex.

greg0ire · 2018-06-13T14:46:31Z

Please remove this phpdoc

I only meant the line with @param

moshenskyDV · 2018-06-13T15:01:12Z

Why param should be removed from PHPDoc?

greg0ire · 2018-06-13T15:35:22Z

What information does it bring? None. It's just noise, and we want people to read our comments, not ignore them because 90% of them are tautologic.

greg0ire · 2018-06-13T15:35:48Z

Use make cs-fix to fix the build.

greg0ire · 2018-06-13T16:02:12Z

Please kindly squash your commits together. If you don't, we'll try to remember to do it for you but it's best if you save us this trouble.

How to do that?

git rebase -i origin/3.x, assuming origin is a git remote that points to this repository, and not your fork. If you're not sure what your remotes are, run git remote -vvv, there should be your fork and the holy/reference/base/origin/whatever-you-call-it repository.
A window will show up with many lines, replace pick with fixup on every line but the first one
Close your editor, git should do its magic, and you should end up with one commit
Use git push --force to overwrite what you already pushed. Don't forget the --force option otherwise git will try to merge both things together.

moshenskyDV · 2018-06-13T20:23:27Z

@greg0ire Thank you for your patience and reviewing this request
Looks like squashed normally.

covex-nn

Just tested:

first, i created a Controller with one action, available on "/" route; then i created a site, then executed bin/console sonata:page:update-core-routes --site=all
in pages tree view a controller action became a root for all other pages. so, everything is good here
then i updated controller action annotation from @Route(path="/") to @Route(path="/test") and executed sonata:page:update-core-routes again
in pages tree view a path to controller action was updated to "/test", no Homepage route created
i cleared a cache and executed sonata:page:update-core-routes again, a new Homepage page appeared, controller action became a child page for Homepage
then i updated controller action annotation from @Route(path="/test") to @Route(path="/"), cleared cache and executed sonata:page:update-core-routes again
from this moment two pages have Url === '/', and this cannot be solved easily

After i added controller action with "/" route into existing project with Homepage page, two pages with '/' route have '/' as Url to. I could not update a page-for-controller-action (there was a error about Url), and when i deleted Homepage, all pages were deleted too =)

moshenskyDV · 2018-06-14T16:49:26Z

@covex-nn i tried to reproduce your comment, and i saw, that we don't check duplicating pages in config.
If we will create something like this in route config:

contact:
    resource: "@ContactBundle/Resources/config/routing.yml"
    prefix:   /contact
contact2:
    resource: "@Contact2Bundle/Resources/config/routing.yml"
    prefix:   /contact

System will create 2 pages with same route, and we can not edit any of this page in admin panel (we will see error about already used URL).

So I think - we need to add check in this file for routes duplicates inside this loop (RoutePageGenerator.php):

// Iterate over declared routes from the routing mechanism
        foreach ($this->router->getRouteCollection()->all() as $name => $route) {

What do you think, guys? It should to be fixed in this PR, or will better to create new PR for this?

rande · 2018-10-11T08:06:37Z

@sonata-project/ekino can you review this PR ?

core23 · 2018-10-11T15:26:49Z

src/Route/RoutePageGenerator.php

+     *
+     * @return object root page
+     */
+    public function createRootPage(SiteInterface $site)


private or at least final

moshenskyDV · 2018-10-27T11:44:26Z

Changed access modifier. Any update of this?
(I not understood what coveralls wants from me, other tests passed)

src/Route/RoutePageGenerator.php

OskarStark · 2019-01-11T12:22:06Z

Can you please rebase your PR?

SonataCI · 2019-01-11T12:30:48Z

Could you please rebase your PR and fix merge conflicts?

moshenskyDV · 2019-01-12T12:37:43Z

Hi. I did. Unfortunately i have no time to test it manually.

greg0ire

We just dropped php < 7.1, you can use type hints now.

src/Route/RoutePageGenerator.php

core23 · 2019-03-08T20:16:18Z

Thanks @moshenskyDV 👍

greg0ire added the patch label Jun 11, 2018

greg0ire reviewed Jun 11, 2018

View reviewed changes

core23 requested changes Jun 12, 2018

View reviewed changes

greg0ire reviewed Jun 12, 2018

View reviewed changes

greg0ire previously approved these changes Jun 13, 2018

View reviewed changes

moshenskyDV dismissed greg0ire’s stale review via b08259b June 13, 2018 20:17

moshenskyDV force-pushed the 3.x branch from 390d968 to b08259b Compare June 13, 2018 20:17

greg0ire previously approved these changes Jun 13, 2018

View reviewed changes

greg0ire requested review from core23 and a team June 13, 2018 21:01

covex-nn reviewed Jun 14, 2018

View reviewed changes

core23 reviewed Oct 11, 2018

View reviewed changes

moshenskyDV dismissed greg0ire’s stale review via e986adc October 19, 2018 18:44

moshenskyDV force-pushed the 3.x branch 3 times, most recently from ea9fd8a to cddc340 Compare October 19, 2018 19:50

moshenskyDV mentioned this pull request Oct 19, 2018

Allow hybrid page on root hierarchy #1016

Closed

core23 requested changes Oct 27, 2018

View reviewed changes

src/Route/RoutePageGenerator.php Outdated Show resolved Hide resolved

moshenskyDV force-pushed the 3.x branch from bcc61bc to 12e1beb Compare October 30, 2018 10:07

core23 previously approved these changes Oct 30, 2018

View reviewed changes

OskarStark requested a review from greg0ire October 30, 2018 12:58

greg0ire reviewed Oct 30, 2018

View reviewed changes

src/Route/RoutePageGenerator.php Outdated Show resolved Hide resolved

moshenskyDV dismissed core23’s stale review via 1266f1b October 31, 2018 08:21

moshenskyDV force-pushed the 3.x branch from 1266f1b to 1846ee6 Compare October 31, 2018 08:22

core23 previously approved these changes Nov 1, 2018

View reviewed changes

SonataCI added the pending author label Jan 11, 2019

moshenskyDV dismissed core23’s stale review via 42b7eb9 January 12, 2019 11:10

SonataCI removed the pending author label Jan 12, 2019

moshenskyDV force-pushed the 3.x branch 3 times, most recently from a385ce6 to 5f8e133 Compare January 12, 2019 12:07

greg0ire requested changes Jan 12, 2019

View reviewed changes

src/Route/RoutePageGenerator.php Outdated Show resolved Hide resolved

src/Route/RoutePageGenerator.php Outdated Show resolved Hide resolved

src/Route/RoutePageGenerator.php Outdated Show resolved Hide resolved

590 duplicate route merge conflict

57f4f1a

moshenskyDV force-pushed the 3.x branch from be1b9de to 57f4f1a Compare January 13, 2019 08:57

core23 approved these changes Jan 13, 2019

View reviewed changes

core23 requested a review from greg0ire March 7, 2019 18:12

greg0ire approved these changes Mar 8, 2019

View reviewed changes

core23 merged commit 1a291b3 into sonata-project:3.x Mar 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

#590 duplicate root route when it in config #992

#590 duplicate root route when it in config #992

moshenskyDV commented Jun 11, 2018 •

edited

moshenskyDV commented Jun 11, 2018

greg0ire Jun 11, 2018

greg0ire commented Jun 11, 2018

core23 Jun 12, 2018

moshenskyDV commented Jun 12, 2018

greg0ire Jun 12, 2018 •

edited

greg0ire Jun 12, 2018

greg0ire commented Jun 13, 2018

moshenskyDV commented Jun 13, 2018

greg0ire commented Jun 13, 2018

greg0ire commented Jun 13, 2018

greg0ire commented Jun 13, 2018

moshenskyDV commented Jun 13, 2018

covex-nn left a comment

moshenskyDV commented Jun 14, 2018 •

edited

rande commented Oct 11, 2018

core23 Oct 11, 2018

moshenskyDV commented Oct 27, 2018

OskarStark commented Jan 11, 2019

SonataCI commented Jan 11, 2019

moshenskyDV commented Jan 12, 2019

greg0ire left a comment

core23 commented Mar 8, 2019

#590 duplicate root route when it in config #992

#590 duplicate root route when it in config #992

Conversation

moshenskyDV commented Jun 11, 2018 • edited

Changelog

Fixed

Security

moshenskyDV commented Jun 11, 2018

greg0ire Jun 11, 2018

Choose a reason for hiding this comment

greg0ire commented Jun 11, 2018

core23 Jun 12, 2018

Choose a reason for hiding this comment

moshenskyDV commented Jun 12, 2018

greg0ire Jun 12, 2018 • edited

Choose a reason for hiding this comment

greg0ire Jun 12, 2018

Choose a reason for hiding this comment

greg0ire commented Jun 13, 2018

moshenskyDV commented Jun 13, 2018

greg0ire commented Jun 13, 2018

greg0ire commented Jun 13, 2018

greg0ire commented Jun 13, 2018

moshenskyDV commented Jun 13, 2018

covex-nn left a comment

Choose a reason for hiding this comment

moshenskyDV commented Jun 14, 2018 • edited

rande commented Oct 11, 2018

core23 Oct 11, 2018

Choose a reason for hiding this comment

moshenskyDV commented Oct 27, 2018

OskarStark commented Jan 11, 2019

SonataCI commented Jan 11, 2019

moshenskyDV commented Jan 12, 2019

greg0ire left a comment

Choose a reason for hiding this comment

core23 commented Mar 8, 2019

moshenskyDV commented Jun 11, 2018 •

edited

greg0ire Jun 12, 2018 •

edited

moshenskyDV commented Jun 14, 2018 •

edited