Skip to content

Change HTML output according to HTML -- Living Standard spec #1301

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

Closed
wants to merge 1 commit into from

Conversation

Kubo2
Copy link
Contributor

@Kubo2 Kubo2 commented May 22, 2015

Supersedes #963.

Any thoughts what else could be included?

} else {
php_info_print(PHP_LOGO_DATA_URI "\" alt=\"PHP logo\" /></a>");
}
php_info_print("<a href=\"http://www.php.net/\"><img border=\"0\" src=\"");
Copy link
Contributor

Choose a reason for hiding this comment

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

You didn't change border to a style property here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for noticing, my mistake. I think I will better rewrite phpinfo()'s output more heavily, to use more stylesheets instead of obsolete attrs etc. and target this to 7.1.

Choose a reason for hiding this comment

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

Speaking of using stylesheets. highlight_*() from what I can see in the commit it is now using the style attr. If you are targetting 7.1 then you could use classes instead and use http://caniuse.com/#feat=style-scoped (provided there is enough browser support by then).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

(provided there is enough browser support by then).

Sadly there isn't and seems there won't be support for that in the near
future (additionaly, Chrome already removed it). But it doesn't matter at
all, there's no problem for stylesheets to have scoped attr with (possible)
forward compatibility and additionaly all classNames prefixed e.g. with php-
.

What's your opinion on this?

Thanks,
Kubo2

2015-05-27 0:06 GMT+02:00 Orion notifications@github.com:

In ext/standard/info.c
#1301 (comment):

@@ -637,12 +640,14 @@ PHPAPI void php_print_info(int flag)
the_time = time(NULL);
ta = php_localtime_r(&the_time, &tmbuf);

  •        php_info_print("<a href=\"http://www.php.net/\"><img border=\"0\" src=\"");
    
  •       if (ta && (ta->tm_mon==3) && (ta->tm_mday==1)) {
    
  •           php_info_print(PHP_EGG_LOGO_DATA_URI "\" alt=\"PHP logo\" /></a>");
    
  •       } else {
    
  •           php_info_print(PHP_LOGO_DATA_URI "\" alt=\"PHP logo\" /></a>");
    
  •       }
    
  •       php_info_print("<a href=\"http://www.php.net/\"><img border=\"0\" src=\"");
    

Speaking of using stylesheets. highlight__() from what I can see in the
commit it is now using the style attr. If you are targetting 7.1 then you
could use classes instead and use http://caniuse.com/#feat=style-scoped *(provided
there is enough browser support by then)_.


Reply to this email directly or view it on GitHub
https://github.com/php/php-src/pull/1301/files#r31084860.

@krakjoe
Copy link
Member

krakjoe commented Jan 7, 2017

@Kubo2 I think this needs splitting: I don't like tests changing so that lengths of strings that are known, can now be any length, and this PR is doing that too much, I don't see a good reason for that. The length of the strings are still known, because you are matching against them.

While I think it is unlikely that anyone is relying on the form of markup, it is possible, and so I can't see a reasonable way to merge this into 7 branches, since it touches so much markup.

I'm going to close this PR, and suggest that you push forward for this at a later time, at the boundary of a major version. I'm aware you done this in time for 7.0, but there is nothing I can do to rewrite the history of what has already happened, unfortunately.

@krakjoe krakjoe closed this Jan 7, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants