Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Modernised phpinfo HTML somewhat #156

Closed
wants to merge 7 commits into from

Conversation

hikari-no-yume
Copy link
Contributor

No description provided.

smalyshev and others added 7 commits August 7, 2012 22:59
* bug40459:
  News for bug#40459
  fix bug #40459 - make all stream funcs that create object call ctor
* PHP-5.4:
  News for bug#40459
  fix bug #40459 - make all stream funcs that create object call ctor
- html5 doctype <!doctype html>
- removed unnecessary <head>, <html> and <body>
- replaced some box table with div
- used CSS for some inline table styles
- closer to validating
@laruence
Copy link
Member

doesn't it already be merged by @nikic?

@nikic
Copy link
Member

nikic commented Aug 14, 2012

@laruence I only merged @reeze commit fixing an issue in phpcredits() that made a test fail. I didn't merge anything else.

@morrisonlevi
Copy link
Contributor

I'd have to look through the actual HTML before approving something like this, but are we for this in principle? Perhaps someone built tools on the phpinfo output? I hope not, but I figured I should ask.

php_info_print("<!DOCTYPE html PUBLIC \"-//W3C//DTD XHTML 1.0 Transitional//EN\" \"DTD/xhtml1-transitional.dtd\">\n");
php_info_print("<html xmlns=\"http://www.w3.org/1999/xhtml\">");
php_info_print("<head>\n");
php_info_print("<!doctype html>\n<meta charset=utf-8>");

Choose a reason for hiding this comment

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

  • DOCTYPE is usually uppercase for bc, but html5 does not care either way.
  • Missing <html lang="en"> ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just use <!doctype> because it looks nicer imo. I don't think it matters.

Also I don't think lang=en matters. If you mean the html tag, that can be omitted and is perfectly valid html (and for simple header pages like this, cleaner imo)

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we should drop html tag. Maybe it's allowed by the standard, but many tools would assume it's there, why make their lives harder?

@andrerom
Copy link

The topic branch php_info_html5 seems to contain un related commits, like 128a4bb, c98a51a and 4db70fd.
Branch could be cleaned up and rebased against upstream master (this), pull request will then be automatically updated

@hikari-no-yume
Copy link
Contributor Author

@andrerom Those commits didn't used to show up here, I think... was master reverted or something?

php_info_print_style(TSRMLS_C);
php_info_print("<title>phpinfo()</title>");
php_info_print("<meta name=\"ROBOTS\" content=\"NOINDEX,NOFOLLOW,NOARCHIVE\" />");
php_info_print("</head>\n");
php_info_print("<body><div class=\"center\">\n");
php_info_print("<div class=\"center\">\n");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why body tag is dropped?

Copy link

Choose a reason for hiding this comment

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

For file size optimization and scannability purposes, consider omitting optional tags.

http://google-styleguide.googlecode.com/svn/trunk/htmlcssguide.xml?showone=Optional_tags#Optional_tags

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep. I do this with all my HTML, it's much simpler, and doesn't really do that much.

Copy link
Contributor

Choose a reason for hiding this comment

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

@bojieli Neither reason is applicable here. What is applicable is writing tools that can easily read this information.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This isn't really supposed to be machine-readable, that's the text format's job.

Copy link
Contributor

Choose a reason for hiding this comment

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

@TazeTSchnitzel Maybe I'm the only one who thinks dropping the body tag is a terrible idea. I've voiced my opinion and that's all I can do. I do agree that the text-format is designed to be human readable, but that doesn't mean we should rape our HTML.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@morrisonlevi rape? It's perfectly valid in HTML5. We aren't dealing with XHTML, here.

Copy link
Contributor

Choose a reason for hiding this comment

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

To be clear: I am all for updating the output, but I see absolutely no reason to approve this pull request. HTML5 give absolutely no benefit over what we already have. HTML5 is not even finalized.

My previous comment was a little harsh. I meant it that way because I feel strongly about this, but I would like to say I do not intend for it to be argumentative. I merely want to express my opinion without it being misunderstood.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This pull request does more than use HTML5. It uses tables less, removes some inline styling, etc, But you're right in that it adds almost nothing, so I'll close it.

@morrisonlevi
Copy link
Contributor

Honestly, any changes OUGHT to be XML serializable. Let's not be lazy just because HTML5 permits it.

@andrerom
Copy link

+1 on xhtml5

@DevNerd
Copy link

DevNerd commented Aug 22, 2012

+2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants