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

Display the Welcome Page when there is no homepage defined #26041

Closed
wants to merge 3 commits into
base: 3.4
from

Conversation

Projects
None yet
@javiereguiluz
Copy link
Member

javiereguiluz commented Feb 4, 2018

Q A
Branch? 3.4
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? yes
Fixed tickets symfony/symfony-docs#9178
License MIT
Doc PR -

In 3.4 we added a trick to display the Welcome Page when the user browses / and there are no routes defined. However, when using the website-skeleton (which is what most newcomers use ... and they are the ones that mostly need the "Welcome Page") the premise about "no routes are defined" is never true and the Welcome Page is never shown (see symfony/symfony-docs#9178 for one of the multiple error reports we've received).

So, I propose to make this change to always define the "Welcome Page" as the fallback:

  • If no routes are defined for /, the Welcome Page is displayed.
  • If there is a route defined for /, this code will never be executed because it's the last condition of the routing matcher.
@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 4, 2018

I'm +1 on the intend.
The UrlMatcher class also needs an update, and the corresponding tests I suppose.

@sroze

This comment has been minimized.

Copy link
Member

sroze commented Feb 4, 2018

Definitely 👍

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Feb 4, 2018

Hey Javier, I like your proposal and in the same direction we might need to make some additional changes:

The phrase ... and you haven't configured any URLs. could be incomplete now. What about finishing this statement with ... at root path. or something similar?

You're seeing this message because you have debug mode enabled and you haven't configured any URLs.

According to your idea, this part should also be updated:

if (0 === count($this->routes) && '/' === $pathinfo) {
throw new NoConfigurationException();
}

Related fixtures that require updating (files):

array($collection, 'url_matcher1.php', array()),
array($redirectCollection, 'url_matcher2.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')),
array($rootprefixCollection, 'url_matcher3.php', array()),
array($headMatchCasesCollection, 'url_matcher4.php', array()),
array($groupOptimisedCollection, 'url_matcher5.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')),
array($trailingSlashCollection, 'url_matcher6.php', array()),
array($trailingSlashCollection, 'url_matcher7.php', array('base_class' => 'Symfony\Component\Routing\Tests\Fixtures\RedirectableUrlMatcher')),

In the other hand, I have a concern now about the name/description of this exception, because it does not fit his new scenario, i.e. it'll be thrown even with routes configuration:

/**
* Exception thrown when no routes are configured.
*
* @author Yonel Ceruto <yonelceruto@gmail.com>
*/
class NoConfigurationException extends ResourceNotFoundException
{
}

Do you think we can add another exception NoRootConfiguration extends NoConfiguration and throw it instead? In that case we might need deprecate the NoConfiguration class or use it only when '' === $code:

$code .= "        if ('/' === \$pathinfo) {\n";
if ('' === $code) {
    $code .= "            throw new Symfony\Component\Routing\Exception\NoConfigurationException();\n";
} else {
    $code .= "            throw new Symfony\Component\Routing\Exception\NoRootConfigurationException();\n";
}
$code .= "        }\n";

@nicolas-grekas nicolas-grekas added this to the 3.4 milestone Feb 5, 2018

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 11, 2018

NoRootConfiguration extends NoConfiguration

I think it doesn't matter much, let's keep things simple IMHO

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Feb 19, 2018

Status: needs work

@ChrisWMichel

This comment has been minimized.

Copy link

ChrisWMichel commented Feb 23, 2018

So is there a quick fix?

@sunnz

This comment has been minimized.

Copy link

sunnz commented Mar 6, 2018

I found this affects 4.0 too. (I just started following the getting started guide with 4.0.)

But I see that this is about the 3.4 branch and the other 4.0 pull requests and issues are closed... so I am wondering if this fix will apply to 4.x too?

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

javiereguiluz commented Mar 9, 2018

@yceruto I'm sorry but I don't know how to make the changes you mentioned in UrlMatcher.php and PhpMatcherDumperTest.php. Can you please give me more details ... or maybe you can take over this PR? Thanks!

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 9, 2018

About the changes in UrlMatcher you need to remove this statement: 0 === count($this->routes) && from here, thus we're removing "there are no routes defined" constraint.

And the fixtures related to PhpMatcherDumperTest.php need to be re-dumped. See the diff here.

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

javiereguiluz commented Mar 11, 2018

@yceruto thanks for your great help! I made the change and tests are now green. Ready for review!

@sstok

sstok approved these changes Mar 11, 2018

@fabpot

fabpot approved these changes Mar 11, 2018

@fabpot

This comment has been minimized.

Copy link
Member

fabpot commented Mar 11, 2018

Thank you @javiereguiluz.

fabpot added a commit that referenced this pull request Mar 11, 2018

bug #26041 Display the Welcome Page when there is no homepage defined…
… (javiereguiluz)

This PR was squashed before being merged into the 3.4 branch (closes #26041).

Discussion
----------

Display the Welcome Page when there is no homepage defined

| Q             | A
| ------------- | ---
| Branch?       | 3.4
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | yes
| Fixed tickets | symfony/symfony-docs#9178
| License       | MIT
| Doc PR        | -

In 3.4 we added a trick to display the Welcome Page when the user browses `/` and there are no routes defined. However, when using the `website-skeleton` (which is what most newcomers use ... and they are the ones that mostly need the "Welcome Page") the premise about *"no routes are defined"* is never true and the Welcome Page is never shown (see symfony/symfony-docs#9178 for one of the multiple error reports we've received).

So, I propose to make this change to always define the "Welcome Page" as the fallback:

* If no routes are defined for `/`, the Welcome Page is displayed.
* If there is a route defined for `/`, this code will never be executed because it's the last condition of the routing matcher.

Commits
-------

5b0d934 Display the Welcome Page when there is no homepage defined

@fabpot fabpot closed this Mar 11, 2018

@settermjd

This comment has been minimized.

Copy link

settermjd commented Mar 12, 2018

Hey @javiereguiluz, will this be ported to the 4.0 branch?

@javiereguiluz

This comment has been minimized.

Copy link
Member Author

javiereguiluz commented Mar 12, 2018

Yes. Pull requests are periodically merged from lower to upper maintained branches, so this will end up in 4.0 and master too. Here is the list of maintained branches: https://symfony.com/roadmap

@mario6097

This comment has been minimized.

Copy link

mario6097 commented Mar 22, 2018

Fedora 26, Symfony 4.0., just installed (by composer create-project symfony/website-skeleton my-project, i update by https://github.com/symfony/demo/blob/master/composer.json too).
I get a "No route found for "GET /" error.".
Do you have a fix? Is there a composer command to install it? Thks

@yceruto

This comment has been minimized.

Copy link
Contributor

yceruto commented Mar 22, 2018

@mario6097 It's already fixed, but it has not been released yet.

This was referenced Apr 3, 2018

@idildin

This comment has been minimized.

Copy link

idildin commented Apr 18, 2018

I use jms/i18n-routing-bundle and update symfony to 3.4.7 and i have problem with this feature.
My config use strategy: prefix and this situation when referring to / must be a redirect to locale url. But showing homepage without redirect /{locale}
This problem is in PhpMatcherDumper.php
@javiereguiluz

@odie2

This comment has been minimized.

Copy link

odie2 commented Apr 26, 2018

Same like @idildin.
I just updated Symfony from 3.4.6 and now can't get default locale prefix redirect, while it was working like a charm before.

composer.json

"jms/i18n-routing-bundle": "2.0.x-dev",

routing.yml

main_index:
    path:     /
    defaults: { _controller: AppBundle:Main:index }
    methods:  [GET]

config.yml

parameters:
    locale: pl
    app.languagesSlugs: [ en, pl ]

framework:
    default_locale:  "%locale%"

# JMS I18N Routing
jms_i18n_routing:
    default_locale: pl
    locales: "%app.languagesSlugs%"
    strategy: prefix

http://localhost:8000/ - Welcome Page (before redirect to http://localhost:8000/pl/)

My question is: should I wait to fix by JMS I18N Routing bundle or by Symfony or is there any workaround to force default language redirect?

@nicolas-grekas

This comment has been minimized.

Copy link
Member

nicolas-grekas commented Apr 26, 2018

Please open dedicated issues instead: you won't get any attention by commenting on closed ones.

@mickaelandrieu

This comment has been minimized.

Copy link
Contributor

mickaelandrieu commented May 29, 2018

Also, this is a BC for everyone involving in progressive migration to Symfony => https://github.com/PrestaShop/PrestaShop/blob/develop/admin-dev/index.php#L87

I can't imagine what motivated you to change the behavior in an LTS :/

Any workaround?

EDIT: nevermind, it's already fixed with the latest 3.4, thanks :)

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