8000 [WebProfilerBundle] Fixed profiler toolbar icons for XHTML. by rafalwrzeszcz · Pull Request #9877 · symfony/symfony · GitHub
[go: up one dir, main page]
Skip to content

[WebProfilerBundle] Fixed profiler toolbar icons for XHTML. #9877

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
Closed
wants to merge 2 commits into from
Closed

[WebProfilerBundle] Fixed profiler toolbar icons for XHTML. #9877

wants to merge 2 commits into from

Conversation

rafalwrzeszcz
Copy link
Contributor
Q A
Bug fix? yes
New feature? no
BC breaks? no
Deprecations? no
Tests pass? n/a
Fixed tickets n/a
License MIT
Doc PR n/a

Profiler page itself is entirely autonomic, but toolbar is not - unfortunately currently it doesn't work in XHTML documents :(.

@wouterj
Copy link
Member
wouterj commented Dec 28, 2013

-1 The profiler uses the html 5 doctype, why should we want to support xHTML?

@jakzal
Copy link
Contributor
jakzal commented Dec 28, 2013

@wouterj I guess the issue reveals itself if you use the toolbar with an xhtml site, since the toolbar is injected into your website's html.

@rafalwrzeszcz
Copy link
Contributor Author

@wouterj Like I noted in the message - it's not about whole profiler page, but a toolbar that gets inserted into page, like @jakzal noted. And also HTML5 doctype is same as XHTML5 doctype btw ;).

@wouterj
Copy link
Member
wouterj commented Dec 28, 2013

@rafalwrzeszcz sorry, I always forget that the toolbar isn't loaded in an iframe. And no, xHTML5 does not exists, not even xHTML2 😉

@fabpot
Copy link
Member
fabpot commented Dec 28, 2013

To be precise, the toolbar is loaded via an Ajax call into the current document.

@rafalwrzeszcz
Copy link
Contributor Author

Yes, but it's then inserted directly into document tree with innerHTML (of containing <div>) and browser parses it accordingly to current document type, which, in case of XHTML, is XML and bum... we have a syntax error :(.

@Tobion
Copy link
Contributor
Tobion commented Dec 29, 2013

👍 but you should make all pages xhtml compatible. Its missing in Profiler views: admin.html.twig, base.html.twig, header.html.twig, layout.html.twig, search.html.twig. And also some Collector views.

@fabpot
Copy link
Member
fabpot commented Dec 29, 2013

@rafalwrzeszcz Can you add a space before the /?
@Tobion No, we don't want to make the profiler itself XHTML compatible, just the toolbar that can be embedded into a non-HTML5 page.

@Tobion
Copy link
Contributor
Tobion commented Dec 29, 2013

XHTML5 is just a more strict version of HTML5 (xml compatible). I didn't say to return the xhtml mime type. We should just try to make the syntax of the profiler also xhtml compatible for consistency. Otherwise we could also remove many end tags from the profiler because

<ul>
<li>Foo
<li>Bar
</ul>

is also valid HTML5.

@rafalwrzeszcz
Copy link
Contributor Author

@Tobion Yes, but (1) I think it's out of scope of this PR (it's just about bugix) and (2) it would produce excessive diff without any real benefit.

@fabpot
Copy link
Member
fabpot commented Jan 6, 2014

@rafalwrzeszcz agreed. As this is a bug fix, can you submit a PR on 2.3 instead?

This was referenced Jan 6, 2014
Closed
Closed
@rafalwrzeszcz
Copy link
Contributor Author

#9948 is the correct one now as it points correct branch.

fabpot added a commit that referenced this pull request Jan 6, 2014
@fabpot
bug #9948 [WebProfilerBundle] Fixed profiler toolbar icons for XHTML.…
6e0848d 
… (rafalwrzeszcz)

This PR was squashed before being merged into the 2.3 branch (closes #9948).

Discussion
----------

[WebProfilerBundle] Fixed profiler toolbar icons for XHTML.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | n/a
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Profiler page itself is entirely autonomic, but toolbar is not - unfortunately currently it doesn't work in XHTML documents :(.

#9877 backported to `2.3` branch.

Commits
-------

296c4d1 [WebProfilerBundle] Fixed profiler toolbar icons for XHTML.
ostrolucky pushed a commit to ostrolucky/symfony that referenced this pull request Mar 25, 2018
@fabpot
bug symfony#9948 [WebProfilerBundle] Fixed profiler toolbar icons for…
edafba5 
… XHTML. (rafalwrzeszcz)

This PR was squashed before being merged into the 2.3 branch (closes symfony#9948).

Discussion
----------

[WebProfilerBundle] Fixed profiler toolbar icons for XHTML.

| Q             | A
| ------------- | ---
| Bug fix?      | yes
| New feature?  | no
| BC breaks?    | no
| Deprecations? | no
| Tests pass?   | n/a
| Fixed tickets | n/a
| License       | MIT
| Doc PR        | n/a

Profiler page itself is entirely autonomic, but toolbar is not - unfortunately currently it doesn't work in XHTML documents :(.

symfony#9877 backported to `2.3` branch.

Commits
-------

296c4d1 [WebProfilerBundle] Fixed profiler toolbar icons for XHTML.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in 5AF8 to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@rafalwrzeszcz @wouterj @jakzal @fabpot @Tobion
0