[RTM] Support for replaying headers #749

Toflar · 2017-04-01T22:03:18Z

Implements our terminal42/header-replay-bundle to support "splitting up" user context provided by cookies (or authorization header or whatever you like) into whatever headers you like that are then replayed as master request onto the application so you can vary on them. Please read the whole README of the header-replay-bundle to understand the concept.

Brings back the support for the mobile page layout because we can now Vary on it
Always show the "live" content when logged in

ToDo's

Unit tests for the two listeners
Wait for the FOSHttpCache and FOSHttpCacheBundle to be released as 2.0.0 stable so I can release terminal42/header-replay-bundle 1.0.0 stable too

Adjust your AppCache according to the README. PR for the Managed Edition: contao/manager-bundle#26

aschempp · 2017-04-02T07:54:49Z

composer.json

@@ -122,5 +124,7 @@
            "dev-develop": "4.4.x-dev"
        },
        "contao-manager-plugin": "Contao\\CoreBundle\\ContaoManager\\Plugin"
-    }
+    },
+    "minimum-stability": "dev",


This should not be necessary if you suffix the requirement with @dev. If that's an issue with an included package, simply require it in the root as well (with @dev).

aschempp · 2017-04-02T07:58:26Z

src/EventListener/HeaderReplay/PageLayoutListener.php

+        }
+
+        $headers = $event->getHeaders();
+        $headers->set('Contao-Page-Layout', $mobile ? 'mobile' : 'desktop');


I would suggest to call this Contao-Mobile-Layout with true only value or the header not being present. Also, creating a $headers variable is fairly unnecessary here, but I guess you're aware of that. Just a matter of choice :D

I like my version better. This applies to both, the header name and the variable.

The idea was that the header would not need to be present for 90% of pages (if not in mobile mode). Also, the Vary would only be needed if the current page actually has a mobile layout.

We could introduce new layout types using the same header with @Toflar's variant. Therefore I am also in favor of his approach.

aschempp · 2017-04-02T08:00:00Z

src/Resources/config/listener.yml

+        arguments:
+            - "%contao.security.disable_ip_check%"
+        tags:
+            - { name: kernel.event_listener, event: t42.header_replay, method: onReplay }


as far as I remember Symfony events, the correct way to name the method would be according to the event name. So t42.header_replay would result in an onHeaderReplay method. Also, if you follow that convention, you do not need to name the method (it will be discovered automatically).

On a side note, I think the event should be called terminal42.header_replay to not introduce another "namespace".

This would look shit, which is why I decided not to do it. onTerminal42HeaderReplay() is not a nice method name at all.

it would be onHeaderReplay. The key is not used. Think of kernel.response => onResponse

No, it's onKernelResponse.

Toflar · 2017-04-02T08:05:33Z

No. It has to be present due to dependencies. I know about the @dev setting.

aschempp · 2017-06-12T14:56:39Z

src/ContaoManager/Plugin.php

@@ -43,7 +44,7 @@ class Plugin implements BundlePluginInterface, RoutingPluginInterface
    public function getBundles(ParserInterface $parser)
    {
        return [
-            BundleConfig::create(KnpMenuBundle::class),


sure this is correct?

Nope, this is a rebase-bug! Fixed in 9fdb93e.

aschempp · 2017-06-12T15:00:26Z

src/EventListener/HeaderReplay/PageLayoutListener.php

+    {
+        $request = $event->getRequest();
+
+        $this->framework->initialize();


pretty sure we should check for a Contao scope before booting the framework on EVERY request?

On every request with a Cookie or Authorization header, yeah. We could do that? How do I do this now?

No I mean only for routes with _scope=frontend.

aschempp · 2017-06-12T15:07:59Z

src/EventListener/HeaderReplay/BackendSessionListener.php

+     */
+    public function onReplay(HeaderReplayEvent $event)
+    {
+        $request = $event->getRequest();


Same applies here. The event listener should only be triggered on $request->attributes->get('_scope') === 'backend' (actually use the constant!)

…espectively

Toflar · 2017-06-12T15:46:56Z

Scope checks implemented.

leofeyer

Besides my two comments and a lot of missing phpDocs the PR looks good to me. 😄

leofeyer · 2017-06-13T09:11:43Z

src/EventListener/HeaderReplay/PageLayoutListener.php

+                case 'desktop':
+                    $mobile = false;
+            }
+        }


We could avoid initializing the framework if there is a TL_VIEW cookie:

if ($request->cookies->has('TL_VIEW')) { $mobile = 'mobile' === $request->cookies->get('TL_VIEW'); } else { $this->framework->initialize(); $mobile = $this->framework->getAdapter(Environment::class)->get('agent')->mobile; }

leofeyer · 2017-06-13T09:13:02Z

src/Resources/contao/classes/FrontendTemplate.php

-		// TODO: Add support for proxies so they can vary on member context and page layout
-		if (FE_USER_LOGGED_IN === true || BE_USER_LOGGED_IN === true || $objPage->isMobile || $objPage->protected || $this->hasAuthenticatedBackendUser())
+		// Vary on page layout
+		$response->setVary(['Contao-Page-Layout'], false);


This is legacy code, so we must not use the short array notation.

Toflar · 2017-06-13T09:32:29Z

@leofeyer comments addressed.

…s (see #749)

aschempp reviewed Apr 2, 2017

View reviewed changes

leofeyer assigned Toflar Apr 5, 2017

leofeyer added the feature label Apr 5, 2017

leofeyer added this to the 4.4.0 milestone Apr 5, 2017

leofeyer added the depends on other label Apr 22, 2017

Toflar mentioned this pull request Apr 28, 2017

[RFC] Removed proxy cache killing varying on User-Agent #743

Merged

leofeyer force-pushed the develop branch 6 times, most recently from cd4aa42 to 51a60f7 Compare May 12, 2017 11:08

leofeyer mentioned this pull request May 18, 2017

Disable AppCache for Active Backend Users contao/manager-bundle#21

Closed

leofeyer force-pushed the develop branch 3 times, most recently from cf5c60f to fbee6c7 Compare May 19, 2017 14:57

Toflar changed the base branch from develop to release/4.4.0 June 12, 2017 11:54

leofeyer force-pushed the release/4.4.0 branch from b3ac62e to d68e371 Compare June 12, 2017 11:54

Toflar added 3 commits June 12, 2017 16:31

WIP

8613831

Added unit tests for the backend session listener

393e046

Added unit tests for page layout listener

1fe7aaf

leofeyer removed the depends on other label Jun 12, 2017

Toflar changed the title ~~[WIP] Support for replaying headers~~ [RTM] Support for replaying headers Jun 12, 2017

Not needed here

6f29f61

leofeyer requested a review from ausi June 12, 2017 14:49

aschempp reviewed Jun 12, 2017

View reviewed changes

Fixed rebase bug

9fdb93e

aschempp reviewed Jun 12, 2017

View reviewed changes

Toflar added 3 commits June 12, 2017 17:21

Fixed event configuration

ba768e4

Fixed tests

b302245

Added scope matcher to only check on front end or back end requests r…

ac60724

…espectively

Fixed unit tests

184b445

aschempp approved these changes Jun 12, 2017

View reviewed changes

leofeyer requested changes Jun 13, 2017

View reviewed changes

Toflar added 2 commits June 13, 2017 11:24

Do not initialize framework if not needed

46db3f6

CS

bee3ce2

leofeyer added 3 commits June 13, 2017 13:40

Fix the coding style.

7894435

Fix the tests.

7b19aa8

Fix the tests.

cc88bd3

leofeyer approved these changes Jun 13, 2017

View reviewed changes

leofeyer merged commit b21599c into contao:release/4.4.0 Jun 13, 2017

leofeyer mentioned this pull request Jun 13, 2017

Probleme mit Browser Cache contao/core#3184

Closed

Toflar deleted the feature/replay-headers branch June 13, 2017 12:42

leofeyer added a commit that referenced this pull request Sep 30, 2019

Prevent regular users from enabling the template editor for themselve…

8286bf5

…s (see #749)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RTM] Support for replaying headers #749

[RTM] Support for replaying headers #749

Toflar commented Apr 1, 2017 •

edited

Loading

aschempp Apr 2, 2017

aschempp Apr 2, 2017

Toflar Apr 4, 2017

aschempp Apr 4, 2017

discordier Apr 4, 2017

aschempp Apr 2, 2017

Toflar Apr 4, 2017

aschempp Apr 4, 2017

Toflar Apr 4, 2017

Toflar commented Apr 2, 2017 via email

aschempp Jun 12, 2017

Toflar Jun 12, 2017

aschempp Jun 12, 2017

Toflar Jun 12, 2017

aschempp Jun 12, 2017

aschempp Jun 12, 2017

Toflar commented Jun 12, 2017

leofeyer left a comment

leofeyer Jun 13, 2017

leofeyer Jun 13, 2017

Toflar commented Jun 13, 2017

[RTM] Support for replaying headers #749

[RTM] Support for replaying headers #749

Conversation

Toflar commented Apr 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toflar commented Apr 2, 2017 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toflar commented Jun 12, 2017

leofeyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Toflar commented Jun 13, 2017

Toflar commented Apr 1, 2017 •

edited

Loading