Skip to content

Use php_info_print_table_header for actual column headers only #9485

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

Merged
merged 1 commit into from
Sep 6, 2022

Conversation

TimWolla
Copy link
Member

@TimWolla TimWolla commented Sep 4, 2022

Using php_info_print_table_header() for "Foo: bar" looks odd and out of place,
because the whole line is colored. It is also questionable from a HTML
semantics point of view, because it does not described the columns that follow.

The use of this across extensions is inconsistent. It was part of the skeleton,
but ext/date or ext/json already use a regular row.

@TimWolla
Copy link
Member Author

TimWolla commented Sep 4, 2022

Before:

image

After:

image

iluuu1994
iluuu1994 previously approved these changes Sep 4, 2022
@iluuu1994
Copy link
Member

The regex php_info_print_table_header.*"enabled" does yield quite a few results though. IMO it would make more sense to just make enabled a first column in the table in all cases.

@devnexen
Copy link
Member

devnexen commented Sep 4, 2022

not a bug fix per say but sort of, so could it apply to a release branch, real question ?

@TimWolla
Copy link
Member Author

TimWolla commented Sep 4, 2022

just make enabled a first column in the table in all cases.

You mean "row", instead of "column"?

@iluuu1994
Copy link
Member

You mean "row", instead of "column"?

Yes 🙂

@TimWolla
Copy link
Member Author

TimWolla commented Sep 4, 2022

Ah, nevermind. I understand now. Apparently I did not include all possible extensions in my local build. I've just verified the change by scrolling through the page. I'll also adjust the others.

Using php_info_print_table_header() for "Foo: bar" looks odd and out of place,
because the whole line is colored. It is also questionable from a HTML
semantics point of view, because it does not described the columns that follow.

The use of this across extensions is inconsistent. It was part of the skeleton,
but ext/date or ext/json already use a regular row.
@TimWolla TimWolla changed the title Unify zend_test formatting in phpinfo() Use php_info_print_table_header for actual column headers only Sep 4, 2022
@TimWolla
Copy link
Member Author

TimWolla commented Sep 4, 2022

not a bug fix per say but sort of, so could it apply to a release branch, real question ?

@devnexen Probably not worth it. Without invoking https://xkcd.com/1172 someone might attempt to parse phpinfo(); to extract information.

@iluuu1994 I've made a sweeping change across all uses of php_info_print_table_header. Thanks for pointing this out!

@TimWolla TimWolla requested a review from iluuu1994 September 4, 2022 19:53
@iluuu1994 iluuu1994 dismissed their stale review September 4, 2022 19:54

Changes made since

Copy link
Member

@cmb69 cmb69 left a comment

Choose a reason for hiding this comment

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

This is a nice improvement (although we cannot possibly cater to external extensions). Thank you!

Copy link
Member

@iluuu1994 iluuu1994 left a comment

Choose a reason for hiding this comment

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

LGTM 🙂

@TimWolla TimWolla merged commit 03fd405 into php:master Sep 6, 2022
@TimWolla TimWolla deleted the zend-test-phpinfo-formatting branch September 6, 2022 07:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment