8000 Symfony 4 compatibility by dbu · Pull Request #420 · FriendsOfSymfony/FOSHttpCacheBundle · GitHub
[go: up one dir, main page]
Skip to content

Symfony 4 compatibility #420

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 25 commits into from
Mar 1, 2018
Merged

Symfony 4 compatibility #420

merged 25 commits into from
Mar 1, 2018

Conversation

dbu
Copy link
Contributor
@dbu dbu commented Dec 28, 2017 •
edited
Loading

Symfony 4 compatibility based on this article.

  • make test app kernel loadable with psr-3, add KERNEL_CLASS to phpunit (but keep KERNEL_DIR for testing older versions of symfony)
  • make cache_manager public
  • Update matthiasnoback/symfony-dependency-injection-test to 2.3 (that supports sf 4.0)
  • find solution for fos_http_cache.event_listener.tag, symfony_response_tagger, proxy_client.varnish, - either get access without having to get it from the container, or make it public for the test env only
  • ServiceTest::testCanBeLoaded => guess we should use the container builder instead of the compiled container in that test. most of those services need not be public probably.
  • Make some more services public in testing so that we can test them

@dbu dbu mentioned this pull request Dec 28, 2017
Closed
4 tasks
Tobion
Tobion suggested changes Jan 7, 2018
View reviewed changes
@dbu dbu force-pushed the symfony-4-compatibility branch from f7741b1 to 3cf40ff Compare January 28, 2018 09:04
@dbu dbu force-pushed the symfony-4-compatibility branch from 2bfb179 to 5ff8d54 Compare January 28, 2018 09:20
dbu
dbu commented Jan 30, 2018
View reviewed changes
@dbu dbu force-pushed the symfony-4-compatibility branch from 14b6751 to 3dfbad1 Compare January 31, 2018 09:09
@dbu
Copy link
Contributor Author
dbu commented Jan 31, 2018

the remaining test failures are all about

FOSHttpCacheBundle/src/EventListener/TagListener.php

Line 111 in 9a48750

if (HttpKernelInterface::MASTER_REQUEST === $event->getRequestType()) {
- debugging showed me that it only sees a request that is not a master request. which is odd, as the request is created as a master request... do you have any idea @Tobion? can we maybe look into this at the office today?

@dbu dbu force-pushed the symfony-4-compatibility branch 5 times, most recently from 86fc261 to 04e1069 Compare March 1, 2018 13:23
@dbu dbu force-pushed the symfony-4-compatibility branch from 04e1069 to 00af897 Compare March 1, 2018 13:41
Tobion
Tobion reviewed Mar 1, 2018
View reviewed changes
src/Resources/config/cache_manager_commands.xml

<service id="fos_http_cache.command.refresh_path" class="FOS\HttpCacheBundle\Command\RefreshPathCommand">
<argument type="service" id="fos_http_cache.cache_manager" />
<tag name="console.command"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is missing the "command" attribute compared to the other command. but since you set the static property now, you don't need to specify the command name in the services anymore

Tobion
Tobion reviewed Mar 1, 2018
View reviewed changes
src/Command/InvalidatePathCommand.php Outdated

/**
* If no cache manager is specified explicitly, fos_http_cache.cache_manager
* is automatically loaded.
*
* @param CacheManager|null $cacheManager The cache manager to talk to
* @param string $commandName Name of this command, in case you want to reuse it
* @param string $commandName Deprecated: Do not set this parameter.
*/
public function __construct(CacheManager $cacheManager = null, $commandName = 'fos:httpcache:invalidate:path')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a better deprecation would be to remove the argument and use func_get_arg logic as fallback and triggering a deprecation when it was passed

@dbu dbu merged commit 187362f into master Mar 1, 2018
@dbu dbu deleted the symfony-4-compatibility branch March 1, 2018 15:57
@XWB
Copy link
Member
XWB commented Mar 6, 2018

@dbu Any clue when FOSHttpCacheBundle 2.2 will be released?

@dbu
Copy link
Contributor Author
dbu commented Mar 8, 2018

@XWB i am trying to wrap up some other things like #430 but hope that after that i can release.

one big issue is symfony/symfony#25736 (comment) - if you have time to look into this and maybe come up with a solution, that would be great.

@XWB
Copy link
Member
XWB commented Mar 8, 2018 •
edited
Loading

@dbu Yes we suffered the same issue after upgrading to Symfony 3.4, but we hacked around it in the Varnish config. If maxage is set, we switch the page to public before Varnish caches it:

sub vcl_backend_response {
    if (beresp.http.Cache-Control ~ "s-maxage" && beresp.http.Cache-Control !~ "s-maxage=0")
        set beresp.http.Cache-Control = regsub(beresp.http.Cache-Control, "private", "public");
        set beresp.http.Cache-Control = regsub(beresp.http.Cache-Control, "must\-revalidate, ", "");
        set beresp.http.Cache-Control = regsub(beresp.http.Cache-Control, "max-age=0, ", "");
    }

Later on, we switch back to private:

sub vcl_deliver {
    if (resp.http.Content-Type ~ "text/html") {
        set resp.http.Cache-Control = "private, no-cache";
    }
}

Perhaps @nicolas-grekas has a better solution. Until then, the hack works just fine.

@dbu
Copy link
Contributor Author
dbu commented Apr 21, 2018 •
edited
Loading

FTR: Symfony 4.1 introduced a special header to tell the session listener to not overwrite visibility in symfony/symfony#26681

we use that since #439

@dbu dbu 6DA9 mentioned this pull request Oct 1, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Tobion Tobion Tobion requested changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dbu @XWB @Tobion
0