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

[WebProfilerBundle] Make WDT follow ajax requests if header set #26655

Merged
merged 1 commit into from Apr 29, 2018

Conversation

Projects
None yet
7 participants
@jeffreymb
Contributor

jeffreymb commented Mar 23, 2018

Replaces #22509. I accidentally closed that PR and couldn't get GitHub to recognize when I added more commits to the branch in my fork.

Q A
Branch? master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Tests pass? yes (There are no tests that I could find)
Fixed tickets #15456
License MIT
Doc PR symfony/symfony-docs#...

When the header of an ajax response contains the Symfony-Debug-Toolbar-Replace header with a value of '1' it will automatically update the toolbar with the toolbar with the debug info from the ajax request.

The bulk of the code in the loadToolbar function in the base_js.html.twig file is moved from the toolbar_js.html.twig file and slightly refactored to use the internal functions instead of making external calls. I take no credit/blame for this code. 馃槈

If this could make it into 4.1 there are multiple people that would be very happy!

@nicolas-grekas nicolas-grekas added this to the 4.1 milestone Mar 24, 2018

@nicolas-grekas nicolas-grekas changed the title from Make WDT follow ajax requests if header set to [WebProfilerBundle] Make WDT follow ajax requests if header set Mar 24, 2018

@gharlan

This comment has been minimized.

Contributor

gharlan commented Mar 25, 2018

@jeffreymb tested it again and found a problem. When the ajax page triggers immediately another ajax call (without the -Replace header), the ajax block in toolbar doesn't stop blinking.

It is reproducible for me with these controllers:

    /**
     * @Route("/")
     */
    public function indexAction(): Response
    {
        return new Response(<<<'HTML'
<html>
<head>
</head>
<body>
    <a href="/foo">Load /foo</a>
    <main></main>
    
    <script src="https://code.jquery.com/jquery-3.3.1.min.js"></script>
    <script>
        $('a').click(function (event) {
            event.preventDefault();
            $('main').load('/foo');
        });
    </script>
</body>
</html>
HTML
);
    }

    /**
     * @Route("/foo")
     */
    public function fooAction(): Response
    {
        $response = new Response('
            foo
            <script>$.get("/bar");</script>
        ');
        $response->headers->set('Symfony-Debug-Toolbar-Replace', 1);

        return $response;
    }

    /**
     * @Route("/bar")
     */
    public function barAction(): Response
    {
        return new Response('', 204);
    }

Load index page and click on "Load /foo". After that the ajax block is blinking all the time here.

@jeffreymb

This comment has been minimized.

Contributor

jeffreymb commented Mar 25, 2018

@gharlan This PR doesn't (at this point) touch the code that is relevant to the issue you've discovered. Is this for sure in scope for this PR?

@gharlan

This comment has been minimized.

Contributor

gharlan commented Mar 25, 2018

Without this PR I don't have this issue.

@jeffreymb

This comment has been minimized.

Contributor

jeffreymb commented Mar 27, 2018

@gharlan fixed

@gharlan

This comment has been minimized.

Contributor

gharlan commented Mar 27, 2018

Thanks, it works great! 鉂わ笍

In base_js.html.twig I found this comment:

This file is partially duplicated in TwigBundle/Resources/views/base_js.html.twig. If you
make any change in this file, verify the same change is needed in the other file.

So maybe you have to copy your changes to this file? I'm not sure.

@jeffreymb

This comment has been minimized.

Contributor

jeffreymb commented Mar 27, 2018

I've double checked that none of the changes in this PR are relevant to TwigBundle/Resources/views/base_js.html.twig.

@jeffreymb

This comment has been minimized.

Contributor

jeffreymb commented Apr 16, 2018

Any chance this can make 4.1? I'm not sure what the policy is, but this PR was created before the feature freeze, and actually most of the work done before the 4.0 feature freeze!

@nicolas-grekas

Would be great to have a doc PR also.

@jeffreymb

This comment has been minimized.

Contributor

jeffreymb commented Apr 16, 2018

There is a docs issue: symfony/symfony-docs#8409

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 20, 2018

@@ -5,6 +5,8 @@ CHANGELOG
-----
* added information about orphaned events
* added ability to auto-refresh the toolbar if a `Symfony-Debug-Toolbar-Replace` header
is set and has a value of '1'

This comment has been minimized.

@fabpot

fabpot Apr 22, 2018

Member

extra whitespace at the beginning of the line

This comment has been minimized.

@nicolas-grekas

nicolas-grekas Apr 22, 2018

Member

here is what I propose here:
made the toolbar auto-update with info from ajax reponses when they set the Symfony-Debug-Toolbar-Replace header to 1

I would have done it myself, but you disallowed me from pushing on your fork :)

@@ -5,6 +5,8 @@ CHANGELOG
-----
* added information about orphaned events
* made the toolbar auto-update with info from ajax reponses when they set the
`Symfony-Debug-Toolbar-Replace header to `1`

This comment has been minimized.

@sroze

sroze Apr 22, 2018

Member

Missing ` after Symfony-Debug-Toolbar-Replace

@jeffreymb

This comment has been minimized.

Contributor

jeffreymb commented Apr 27, 2018

I created a doc PR earlier today.

@nicolas-grekas

This comment has been minimized.

Member

nicolas-grekas commented Apr 29, 2018

Thank you @jeffreymb.

@nicolas-grekas nicolas-grekas merged commit e4e591b into symfony:master Apr 29, 2018

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
fabbot.io Your code looks good.
Details

nicolas-grekas added a commit that referenced this pull request Apr 29, 2018

feature #26655 [WebProfilerBundle] Make WDT follow ajax requests if h鈥
鈥ader set (jeffreymb)

This PR was squashed before being merged into the 4.1-dev branch (closes #26655).

Discussion
----------

[WebProfilerBundle] Make WDT follow ajax requests if header set

Replaces #22509. I accidentally closed that PR and couldn't get GitHub to recognize when I added more commits to the branch in my fork.

| Q             | A
| ------------- | ---
| Branch?       | master
| Bug fix?      | no
| New feature?  | yes <!-- don't forget updating src/**/CHANGELOG.md files -->
| BC breaks?    | no
| Deprecations? | no <!-- don't forget updating UPGRADE-*.md files -->
| Tests pass?   | yes (There are no tests that I could find)
| Fixed tickets | #15456
| License       | MIT
| Doc PR        | symfony/symfony-docs#... <!--highly recommended for new features-->

When the header of an ajax response contains the `Symfony-Debug-Toolbar-Replace` header with a value of '1' it will automatically update the toolbar with the toolbar with the debug info from the ajax request.

The bulk of the code in the `loadToolbar` function in the `base_js.html.twig` file is moved from the `toolbar_js.html.twig` file and slightly refactored to use the internal functions instead of making external calls. I take no credit/blame for this code. 馃槈

If this could make it into 4.1 there are multiple people that would be very happy!

Commits
-------

e4e591b [WebProfilerBundle] Make WDT follow ajax requests if header set

javiereguiluz added a commit to symfony/symfony-docs that referenced this pull request May 2, 2018

minor #9692 WDT following AJAX requests (jeffreymb)
This PR was squashed before being merged into the master branch (closes #9692).

Discussion
----------

WDT following AJAX requests

This PR is for #8409 and the functionality in symfony/symfony#26655.

This is my first Doc PR so I'm happy for all the constrictive input there is to give. I'm really hoping this functionality can make 4.1.

Commits
-------

1bbca83 WDT following AJAX requests

@fabpot fabpot referenced this pull request May 7, 2018

Merged

Release v4.1.0-BETA1 #27181

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