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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[WIP] CMF / Dynamic Routing #95

Merged
merged 42 commits into from Jan 7, 2019

Conversation

Projects
None yet
5 participants
@aschempp
Copy link
Contributor

aschempp commented Sep 25, 2018

Continuing from contao/core-bundle#1653

This is a work in progress how to implement the CMF Dynamic Router to the Contao code base. It currently does nothing more than routing, there are no new features like real controllers for pages etc.

Feel free to review and test it, but there's a lot still to be changed 馃槉

TODO:

  • If prepend_locale is enabled and a page without locale is accessed, the visitor was redirected. E.g. https://demo.contao.org/news.html => https://demo.contao.org/en/news.html.
  • If no pages are defined in tl_page, the NoRootPageFoundException should be rendered
  • Because the PrettyErrorScreenHandler only handles Contao routes (_scope=frontend/backend), it is now never triggered on actual 404s.
  • should we add a redirect if root is set to use HTTPS? see #262
  • is / really a valid alternative alias for the first page instead of index?

@aschempp aschempp force-pushed the aschempp:feature/dynamic-router branch from 0a62a3d to 7e9d8ef Sep 25, 2018

christian-kolb pushed a commit to christian-kolb/contao that referenced this pull request Oct 11, 2018

[Installation] Bypass the DB connection setup when using env variable鈥
鈥 (see contao#95)

Description
-----------

Bypass the database connection setup per form if the application is already configured using the env component.

## What it is all about

When using the `symfony/website-skeleton`, you're using the [Dotenv component](http://symfony.com/doc/current/components/dotenv.html) to configure your application. If this is the case, we can basically skip the database setup via the form in the installation bundle, as the dumped `parameters.yml` file will not be included anyway.

## What it does

* [Check for the environment variable DATABASE_URL](https://github.com/sheeep/installation-bundle/blob/fix/allow-env-component-configurations/src/Controller/InstallationController.php#L317), if given, assume the application is configured using the env component.
* [Show a special error message](https://github.com/sheeep/installation-bundle/blob/fix/allow-env-component-configurations/src/Controller/InstallationController.php#L318) if using the env component but the connection to the database failed.
* [Check the given connection](https://github.com/sheeep/installation-bundle/blob/fix/allow-env-component-configurations/src/InstallTool.php#L99-L102). If it doesn't work fall back to the [default behaviour](https://github.com/sheeep/installation-bundle/blob/fix/allow-env-component-configurations/src/InstallTool.php#L106-L128).

## Open points

* [x] Translations. So far, this PR only provides the english translation of the two new string `misconfigured_database_url` and `misconfigured_database_url_explain`.
* [x] Tests

I probably missed quite a few things, so this is RFC.

Commits
-------

f1ead511 Bypass the database connection setup per form if the application is already configured using the env component.
0bd0dc3f Re-use database_could_not_connect translation key
b3f67bc3 Shorten the comments

christian-kolb pushed a commit to christian-kolb/contao that referenced this pull request Oct 11, 2018

[Core] Document the installation process for Symfony Flex (see #1632)
Description
-----------

This is an overhaul of the installation process documentation in the `README.md` file.
There is a few things this documentation relies on:

* [ ] `contao/core-bundle` v4.6 must be released so the [recipe PR](symfony/recipes-contrib#426) can be merged.
* [x] The Installation bundle must support the `DATABASE_URL` environment variable configuration, so [PR contao#95](contao/installation-bundle#95) must be merged

If you try this with a bare bone Symfony  (`composer create-project symfony/website-skeleton`) you will run run into an error, as the `json_manifest_file` does not yet exist.

Be sure to disable the manifest file in `config/assets.yaml`, as you won't be able to run `yarn run encore dev` without adding at least one asset file. But you'd need the manifest file in order to run the `asset()` calls in the error template of Contao.

I'm pretty sure this edge case is out of scope of this documentation, that's why I left it out.

Commits
-------

bef7b2ea Documented the installation process for Symfony Flex.
ffb62003 Fix the wording

@aschempp aschempp force-pushed the aschempp:feature/dynamic-router branch from 17925ba to bb54618 Oct 17, 2018

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Oct 17, 2018

To "successfully" run the functional tests, the following adjustments to core are necessary:

  • Remove exit in Controller::redirect()
  • Replace the routing.yml in functional tests with the one from Resources/config

The tests are successful so far as the following things are up for discussion:

  1. PageRoot::generate() throws a 303 instead of 302 redirect. This code was probably not reached previously because the URL matching already redirected, but Sf routing now simply uses the correct controller.
  2. The alias /.html or /en/.html was previously redirected to the first page. I think that's wrong, it should generate a 404 (Sf routing does that).
  3. The "no root page found" state currently renders a 404 instead of a 500. And probably does not render the correct "information page", because Sf just does not find a matching route. Not sure how we should handle that, because we would need to have a controller for that case.

@contao contao deleted a comment from coveralls Oct 17, 2018

@leofeyer
Copy link
Member

leofeyer left a comment

It seems to me that you have made some changes for the unit tests only, which is the wrong approach IMHO.

@@ -80,9 +75,8 @@ class ContaoFramework implements ContaoFrameworkInterface, ContainerAwareInterfa
*/
private $hookListeners = [];
public function __construct(RequestStack $requestStack, RouterInterface $router, ScopeMatcher $scopeMatcher, string $rootDir, int $errorLevel)
public function __construct(?RequestStack $requestStack /* removed in Contao 4.7 */, RouterInterface $router, ScopeMatcher $scopeMatcher, string $rootDir, int $errorLevel)

This comment has been minimized.

@leofeyer

leofeyer Oct 17, 2018

Member

Why did you comment this instead of removing it?

This comment has been minimized.

@aschempp

aschempp Oct 17, 2018

Contributor

Because it would be a BC break?

This comment has been minimized.

@leofeyer

leofeyer Oct 17, 2018

Member

Hm, the parameter has been removed in Contao 4.7 and your pull request is targeted against Contao 4.7. 馃

This comment has been minimized.

@aschempp

aschempp Oct 17, 2018

Contributor

What do you mean it has been removed?

This comment has been minimized.

@leofeyer

leofeyer Oct 17, 2018

Member

That's what your comment says: /* removed in Contao 4.7 */

This comment has been minimized.

@aschempp

aschempp Oct 18, 2018

Contributor

lol. Yeah, it was removed in this pull request because we now have setRequest. But we can't change the arguments, that's why there is a comment on why the argument is still there.

Show resolved Hide resolved core-bundle/src/Framework/ContaoFramework.php Outdated
\define('TL_START', microtime(true));
}
if (!\defined('TL_ROOT')) {

This comment has been minimized.

@leofeyer

leofeyer Oct 17, 2018

Member

This check should not have to be there, as the framework cannot be initialized twice. Did you add it for the unit tests only?

This comment has been minimized.

@aschempp

aschempp Oct 17, 2018

Contributor

TL_ROOT cannot be initialized if the request is not matched.

Show resolved Hide resolved core-bundle/src/Routing/FrontendLoader.php
Show resolved Hide resolved core-bundle/src/Routing/LegacyRouteProvider.php

@leofeyer leofeyer added this to the 4.7.0 milestone Oct 17, 2018

@aschempp aschempp force-pushed the aschempp:feature/dynamic-router branch from f34f6b5 to 0ee8024 Oct 17, 2018

leofeyer added a commit that referenced this pull request Oct 18, 2018

Adjustments to functional tests (see #133)
Description
-----------

1. Only test for a partial match of the page title. This is forward compatible with Symfony Routing (#95) because the 404 message will not be identical.
2. Pass the server variables to the test client, otherwise they will not appear in `$request`
3. Test HTTP status code before `$_GET` parameters

Commits
-------

1696337 Only test if the page title matches partially
29e5c2b Test status code before the query parameters
fa78149 Actually set the server variables in the functional test client

@leofeyer leofeyer force-pushed the contao:master branch from b641979 to e8ae423 Oct 18, 2018

@aschempp aschempp force-pushed the aschempp:feature/dynamic-router branch from 0ee8024 to f79f6c8 Oct 18, 2018

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Oct 18, 2018

If prepend_locale is enabled and a page without locale is accessed, the visitor was redirected. E.g. https://demo.contao.org/news.html => https://demo.contao.org/en/news.html. Is that intended?

It sure is. I have added a test in 825a5b0.

@ausi

This comment has been minimized.

Copy link
Member

ausi commented Oct 18, 2018

This redirect is currently also triggered for non-existent pages (e.g. https://demo.contao.org/foo.html redirects to https://demo.contao.org/en/foo.html). IMO it would be better to show a 404 page directly instead of the redirect.

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Oct 18, 2018

To "successfully" run the functional tests, the following adjustments to core are necessary:

  • Remove exit in Controller::redirect()
  • Replace the routing.yml in functional tests with the one from Resources/config

Done.

The tests are successful so far as the following things are up for discussion:

  1. PageRoot::generate() throws a 303 instead of 302 redirect. This code was probably not reached previously because the URL matching already redirected, but Sf routing now simply uses the correct controller.

Fine with me.

  1. The alias /.html or /en/.html was previously redirected to the first page. I think that's wrong, it should generate a 404 (Sf routing does that).

Perfectly fine!

  1. The "no root page found" state currently renders a 404 instead of a 500. And probably does not render the correct "information page", because Sf just does not find a matching route. Not sure how we should handle that, because we would need to have a controller for that case.

As discussed in Mumble on October 18th, it would be good to preserve the "no root page found" error if possible, so beginners know why they don't see anything.

@aschempp aschempp force-pushed the aschempp:feature/dynamic-router branch from 4676e63 to dbd506a Oct 19, 2018

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Oct 19, 2018

PR updated to correctly sort/match pages based on the browser accept language. That was not present yet 馃槆

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Oct 19, 2018

As discussed in Mumble on October 18th, it would be good to preserve the "no root page found" error if possible, so beginners know why they don't see anything.

This is possible, however, the PrettyErrorScreenListener does not render at all if the pages are not in scope = frontend/backend (https://github.com/contao/contao/blob/master/core-bundle/src/EventListener/PrettyErrorScreenListener.php#L106). And if the router does not match a page, there is no scope, so it never renders. Can we agree on changing this so the pretty screens are always rendered for HTML output? @sheeep @bytehead

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Oct 19, 2018

I'm not sure about it, but if it is the only solution, I'd rather have it this way than not having it at all.

@aschempp aschempp force-pushed the aschempp:feature/dynamic-router branch from dbd506a to 6941384 Oct 19, 2018

@bytehead

This comment has been minimized.

Copy link
Member

bytehead commented Oct 19, 2018

The PrettyErrorScreenListener is fine for me for routes in Contao scope, but only for them.

Can we agree on changing this so the pretty screens are always rendered for HTML output?

馃憥

@aschempp

This comment has been minimized.

Copy link
Contributor

aschempp commented Oct 22, 2018

The PrettyErrorScreenListener is fine for me for routes in Contao scope, but only for them.

How do you want to know if a 404 (no match found) is Contao route?

@contao contao deleted a comment from aschempp Oct 22, 2018

@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Oct 22, 2018

Yeah, that seems impossible. You can either enable pretty error screens 鈥撀爐hen you will get them everywhere 鈥撀爋r you can disable them 鈥撀爐hen you will not get them anywhere.

@asaage

This comment has been minimized.

Copy link

asaage commented Jan 3, 2019

Can this accomplish correct error-pages for sites with enabled folder-url's?
https://github.com/contao/core-bundle/issues/1624

leofeyer and others added some commits Jan 4, 2019

leofeyer and others added some commits Jan 6, 2019

@leofeyer leofeyer force-pushed the aschempp:feature/dynamic-router branch from 88726a4 to 5983d04 Jan 6, 2019

@leofeyer leofeyer merged commit 79f1135 into contao:master Jan 7, 2019

1 of 2 checks passed

coverage/coveralls Coverage decreased (-1.3%) to 89.646%
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@leofeyer

This comment has been minimized.

Copy link
Member

leofeyer commented Jan 7, 2019

Thanks a lot @aschempp. This is huge! 馃帀

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