Skip to content

Removed Logo GUIDs and replaced with Data URIs and div hidden with JS - #132

Merged
php-pulls merged 5 commits into
php:masterfrom
hikari-no-yume:data_uri_logos
Aug 6, 2012
Merged

Removed Logo GUIDs and replaced with Data URIs and div hidden with JS#132
php-pulls merged 5 commits into
php:masterfrom
hikari-no-yume:data_uri_logos

Conversation

@hikari-no-yume

Copy link
Copy Markdown
Contributor
  • removed php_logo_guid()
  • removed php_egg_logo_guid()
  • removed php_real_logo_guid()
  • removed zend_logo_guid()
  • removed logo GUID handling
  • removed logo GUIDs from source
  • added logo data URIs instead for phpinfo()
  • added credits to phpinfo() page, but hidden by default

- removed php_logo_guid()
- removed php_egg_logo_guid()
- removed php_real_logo_guid()
- removed zend_logo_guid()
- removed logo GUID handling
- removed logo GUIDs from source
- added logo data URIs instead for phpinfo()
- added credits to phpinfo() page, but hidden by default
Comment thread ext/standard/info.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

A small misalign :)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

fixed

@smalyshev

Copy link
Copy Markdown
Contributor

I like the idea, but I am a bit concerned on Javascript thing - it looks like it'd be impossible to see credits in a browser that doesn't have Javascript enabled.

@nikic

nikic commented Jul 14, 2012

Copy link
Copy Markdown
Member

@smalyshev The credits are hidden using JS. So if JS is disabled the credits won't get hidden ;)

@hikari-no-yume

Copy link
Copy Markdown
Contributor Author

No, it's visible by default, it's just hidden by the JavaScript when the page loads. So if you have JS turned off, you can see it fine. If you have it turned on, you'll have to click the link to see it.

Edit: nikic beat me to it :)

@laruence

Copy link
Copy Markdown
Member

Hi:

  1. it's better use a local var hold the result of getElementById
  2. 'use strict' is not necessary here, since we only have less than 10 lines js codes
  3. seems we can only 'display' the credits, but can not 'hidden' it again.

thanks :)

@nikic

nikic commented Jul 15, 2012

Copy link
Copy Markdown
Member

@laruence The use strict doesn't hurt anyone though ^^

@hikari-no-yume

Copy link
Copy Markdown
Contributor Author

If it matters so much I'll clear up the code a little, but 'use strict' is best practise.

@php-pulls

Copy link
Copy Markdown

Comment on behalf of laruence at php.net:

closed

@php-pulls php-pulls closed this Jul 16, 2012
@laruence

Copy link
Copy Markdown
Member

damned, I am testing the pull request list bug, closed this by accident...

@bjori bjori reopened this Jul 16, 2012
@laruence

Copy link
Copy Markdown
Member

@bjori thanks :)

Comment thread ext/standard/info.c Outdated

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Won't this cause strange behavior if somebody is embedding the phpinfo() page in a page already overwriting window.onload? People are embedding phpinfo() in installers or configuration areas for different applications.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They shouldn't be including a full HTML page, that's bound to break
things anyway. They should be using an iframe or a popup. I feel no
sympathy for them if they're doing that, things can break.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

window.onload should just be onload [edited], since window is a reference to the global object.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Huh, what? If you replace window.onload with window you'd overwrite the global scope with said function.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I prefer to be more specific (and hence verbose). I always use, e.g.,
window.onload, window.WebSocket, etc.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

phpinfo and phpcredits aren't the same thing.

Please give me an actual example.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@TazeTSchnitzel I think your Facebook just went wild ;)

@hikari-no-yume hikari-no-yume Aug 11, 2012

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes it did. Last time I trust them with my contact list.

This is very irritating, I apologise.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Under normal circumstances I'd mock you for having a Facebook account in
the first place. But o think you've already seen the error of your ways ;)
On Aug 11, 2012 2:51 PM, "Andrew Faulds" notifications@github.com wrote:

In ext/standard/info.c:

    php_info_print_hr();
  •   php_info_print("<h1><a href=\"");
    
  •   php_info_print_request_uri(TSRMLS_C);
    
  •   php_info_print("?=PHPB8B5F2A0-3C92-11d3-A3A9-4C7B08C10000\">");
    
  •   php_info_print("<script>(function () {\n");
    
  •   php_info_print("    'use strict';\n");
    
  •   php_info_print("    window.onload = function () {\n");
    

Yes it did. Last time I trust them with my contact list. This is very
irritating, I apologise.
-- Sent from Samsung Mobile Andrew Faulds http://ajf.me/ nikic <
notifications@github.com> hat geschrieben: In ext/standard/info.c:
php_info_print_hr(); - php_info_print("

<a href=""); -
php_info_print_request_uri(TSRMLS_C); -
php_info_print("?=PHPB8B5F2A0-3C92-11d3-A3A9-4C7B08C10000">"); +
php_info_print("<script>(function () {\n"); + php_info_print(" 'use
strict';\n"); + php_info_print(" window.onload = function () {\n");
@TazeTSchnitzel https://github.com/TazeTSchnitzel I think your
Facebook just went wild ;) — Reply to this email directly or view it on
GitHub.


Reply to this email directly or view it on GitHubhttps://github.com//pull/132/files#r1357361.

@hikari-no-yume hikari-no-yume Aug 11, 2012

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I thought rejoining Facebook was a good thing.

I was sadly mistaken ;)

@ghost

ghost commented Jul 26, 2012

Copy link
Copy Markdown

+1 for this request

@pilif

pilif commented Jul 28, 2012

Copy link
Copy Markdown
Contributor

You could use window.addEventListener for compatibility with pages that embed the phpinfo() output.

This of course would produce very broken HTML as the phpinfo() output contains a full html page including the root element and a doctype and pretty nonspecific CSS which would certainly clash with the embedding pages.

For these reasons, I would guess that enbedding via an iframe makes more sense anyways - but who knows what people are doing.

@php-pulls

Copy link
Copy Markdown

Comment on behalf of stas at php.net:

merged

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.

10 participants