search

ClassicPress / ClassicPress-v1

A copy of ClassicPress v1.x.
1 stars 1 forks source link

Remove dns-prefetch and assets loaded from s.w.org #206

Closed stefanos82 closed 5 years ago

stefanos82 commented 5 years ago

I have found two places that mention dns-prefetch that points at s.w.org which redirects to WordPress's website.

Also, it can be found in ClassicPress's website as <link rel='dns-prefetch' href='//s.w.org' />.

Is there a particular reason we need it?

Please advise.

Thank you.

nylen commented 5 years ago

Good catch!

$ ack s.w.org
tests/phpunit/tests/formatting/Emoji.php:9: private $png_cdn = 'https://s.w.org/images/core/emoji/11/72x72/';
tests/phpunit/tests/formatting/Emoji.php:10:    private $svn_cdn = 'https://s.w.org/images/core/emoji/11/svg/';
tests/phpunit/tests/http/base.php:16:   var $fileStreamUrl = 'http://s.w.org/screenshots/3.9/dashboard.png';
tests/phpunit/tests/http/base.php:35:       if ( 0 === strpos( $response->get_error_message(), 'stream_socket_client(): unable to connect to tcp://s.w.org:80' ) ) {
tests/phpunit/tests/general/resourceHints.php:34:       $expected = "<link rel='dns-prefetch' href='//s.w.org' />\n";
tests/phpunit/tests/general/resourceHints.php:42:       $expected = "<link rel='dns-prefetch' href='//s.w.org' />\n" .
tests/phpunit/tests/general/resourceHints.php:73:       $expected = "<link rel='dns-prefetch' href='//s.w.org' />\n" .
tests/phpunit/tests/general/resourceHints.php:101:      $expected = "<link rel='dns-prefetch' href='//s.w.org' />\n" .
tests/phpunit/tests/general/resourceHints.php:127:      $expected = "<link rel='dns-prefetch' href='//s.w.org' />\n" .
tests/phpunit/tests/general/resourceHints.php:149:                  "<link rel='dns-prefetch' href='//s.w.org' />\n";
tests/phpunit/tests/general/resourceHints.php:168:                  "<link rel='dns-prefetch' href='//s.w.org' />\n";
tests/phpunit/tests/general/resourceHints.php:185:      $expected = "<link rel='dns-prefetch' href='//s.w.org' />\n";
tests/phpunit/tests/general/resourceHints.php:202:      $expected = "<link rel='dns-prefetch' href='//s.w.org' />\n";
tests/phpunit/tests/general/resourceHints.php:215:      $expected = "<link rel='dns-prefetch' href='//s.w.org' />\n";
tests/phpunit/tests/general/resourceHints.php:250:      $expected = "<link rel='dns-prefetch' href='//s.w.org' />\n" .
tests/qunit/wp-admin/js/widgets/test-media-image-widget.js:13:      testImageUrl = 'http://s.w.org/style/images/wp-header-logo.png';
tests/qunit/wp-admin/js/widgets/test-media-image-widget.js:94:      imageWidgetControlInstance.model.set({ error: false, url: 'http://s.w.org/style/images/wp-header-logo.png' });
tests/qunit/wp-admin/js/widgets/test-media-image-widget.js:98:          equal( imageWidgetControlInstance.$el.find( 'img[src="http://s.w.org/style/images/wp-header-logo.png"]' ).length, 1, 'One image should be rendered' );
src/wp-includes/formatting.php:5036:        'baseUrl' => apply_filters( 'emoji_url', 'https://s.w.org/images/core/emoji/11/72x72/' ),
src/wp-includes/formatting.php:5054:        'svgUrl' => apply_filters( 'emoji_svg_url', 'https://s.w.org/images/core/emoji/11/svg/' ),
src/wp-includes/formatting.php:5176:    $cdn_url = apply_filters( 'emoji_url', 'https://s.w.org/images/core/emoji/11/72x72/' );
src/wp-includes/general-template.php:2888:  $hints['dns-prefetch'][] = apply_filters( 'emoji_svg_url', 'https://s.w.org/images/core/emoji/11/svg/' );
src/wp-admin/about.php:219:             <source media="(max-width: 500px)" srcset="<?php echo 'https://s.w.org/images/core/4.9/banner-mobile.svg'; ?>">
src/wp-admin/about.php:220:             <img src="https://s.w.org/images/core/4.9/banner.svg" alt="">
src/wp-admin/about.php:232:                     <img src="https://s.w.org/images/core/4.9/draft-and-schedule.svg" alt="">
src/wp-admin/about.php:239:                     <img src="https://s.w.org/images/core/4.9/design-preview-links.svg" alt="">
src/wp-admin/about.php:246:                     <img src="https://s.w.org/images/core/4.9/locking.svg" alt="">
src/wp-admin/about.php:253:                     <img src="https://s.w.org/images/core/4.9/prompt.svg" alt="">
src/wp-admin/about.php:269:                     <img src="https://s.w.org/images/core/4.9/syntax-highlighting.svg" alt="">
src/wp-admin/about.php:276:                     <img src="https://s.w.org/images/core/4.9/sandbox.svg" alt="">
src/wp-admin/about.php:283:                     <img src="https://s.w.org/images/core/4.9/warning.svg" alt="">
src/wp-admin/about.php:299:                     <img src="https://s.w.org/images/core/4.9/gallery-widget.svg" alt="">
src/wp-admin/about.php:306:                     <img src="https://s.w.org/images/core/4.9/media-button.svg" alt="">
src/wp-admin/about.php:322:                     <img src="https://s.w.org/images/core/4.9/theme-switching.svg" alt="">
src/wp-admin/about.php:329:                     <img src="https://s.w.org/images/core/4.9/menu-flow.svg" alt="">

So, emoji assets are served from this host, and so are a few things from the wp-admin About page (which we'll remove, see ClassicPress/ClassicPress-v1#262).

Probably the best solution for us for v1 is to bundle the emoji assets with ClassicPress and serve them directly from the install, but I'm open to ideas and contributions here.

See also: https://petitions.classicpress.net/posts/7/remove-emojis-gdpr-friendly

stefanos82 commented 5 years ago

@nylen I was about to suggest the same thing; just bundle them for now and afterwards we can serve them as part of a service that will handle ClassicPress's static content as a whole.

stefanos82 commented 5 years ago

@nylen I have checked with silver searcher ag and have found files that contain WordPress links. I'm going to list just a few of them here and of course I'm pretty sure there are some that could be considered as false-positives, such as links inside comments and so forth; nevertheless, feel free to check them yourself:

The list could go on...

nylen commented 5 years ago

Let's focus on s.w.org for the purpose of this issue, that is nice and specific. If you'd like to create other issues for other specific types of links we should look at, then please do.

stefanos82 commented 5 years ago

Not a bad idea actually. I will do that right now.

johnalarcon commented 5 years ago

A couple of things to decide before proceeding with this.

  1. Where will these images be stored within the local file structure? ../wp-admin/about.php https://s.w.org/images/core/4.9/banner-mobile.svg https://s.w.org/images/core/4.9/banner.svg https://s.w.org/images/core/4.9/draft-and-schedule.svg https://s.w.org/images/core/4.9/design-preview-links.svg https://s.w.org/images/core/4.9/locking.svg https://s.w.org/images/core/4.9/prompt.svg https://s.w.org/images/core/4.9/syntax-highlighting.svg https://s.w.org/images/core/4.9/sandbox.svg https://s.w.org/images/core/4.9/warning.svg https://s.w.org/images/core/4.9/gallery-widget.svg https://s.w.org/images/core/4.9/media-button.svg https://s.w.org/images/core/4.9/theme-switching.svg https://s.w.org/images/core/4.9/menu-flow.svg
  2. Will we be pointing internally (locally) or hosting images? ../wp-includes/formatting.php https://s.w.org/images/core/emoji/11/72x72/ (baseUrl) https://s.w.org/images/core/emoji/11/svg/ (svgUrl) https://s.w.org/images/core/emoji/11/72x72/ ($cdn_url)
  3. Will we repoint or remove the DNS prefetch? ../wp-includes/general-template.php https://s.w.org/images/core/emoji/11/svg/ (DNS prefetch)
stefanos82 commented 5 years ago

My suggestion would be to create a new parent folder named assets inside root directory with sub-folders images/core/ and place them there.

As soon as we set up a service URL, it should point at https://foo-something.org/images/core/....

The reason I have suggested assets is so we can move other types of static files as well, such as CSS, small videos if we like...there are many benefits to get out of it.

nylen commented 5 years ago

Where will these images be stored within the local file structure?

  1. wp-admin/about.php

These were specific to the WP About page. These references have all been removed from the ClassicPress code so there is nothing to do here.

  1. https://s.w.org/images/core/emoji/11/

Let's put these under wp-includes/images/emoji/. I am also curious what the 11 means in those URLs.

  1. Will we repoint or remove the DNS prefetch?

After s.w.org references are removed, we can remove the DNS prefetch too. There's no point in adding a DNS prefetch to the same site you're currently on, as we already know that the browser knows how to reach it :wink:

My suggestion would be to create a new parent folder named assets inside root directory

There are already places for all of these types of assets in the current code structure (images, css, js folders under wp-includes).

I'm definitely not saying the current structure is ideal, but let's think about a more complete reorganization for a future major version (v2 or beyond), and until then, avoid making changes unless absolutely necessary.

senlin commented 5 years ago

I have time to do this, thing is though that the emoji reference URLs are all blocked?

nylen commented 5 years ago

What do you mean by blocked?

senlin commented 5 years ago

The 3 s.w.org URLs are blocked as in:

blocked-sworg

For the following URLs:

nylen commented 5 years ago

Ah. More specifically: they don't have directory indexes enabled there.

There are files included from s.w.org which are valid, though. So part of this task is to find a full list of those files and mirror them.

senlin commented 5 years ago

So part of this task is to find a full list of those files and mirror them.

Guess I will put the Help wanted label back then.

senlin commented 5 years ago

@nylen would it be safe to already remove the references from the tests?

kraftbj commented 5 years ago

I am also curious what the 11 means in those URLs.

Twemoji 11's emoji, which versioning (now) matches the Unicode version upon which it is based. Used to ensure consistent emoji styles are returned and there's no chance of caching delivering a mixed result of emoji styles if we only updated a single set of emoji. Older versions of WordPress defaulted to different versions of Twemoji, which are still available on the CDN as they were.

kraftbj commented 5 years ago

There are files included from s.w.org which are valid, though. So part of this task is to find a full list of those files and mirror them.

https://github.com/twitter/twemoji/tree/gh-pages/2 -- the 72x72 and svg folders there.

nylen commented 5 years ago

Thanks @kraftbj!

So maybe src/wp-includes/images/twemoji/11/72x72/ and src/wp-includes/images/twemoji/11/svg/ is the way to go here.

senlin commented 5 years ago

Question:

After this is merged, this

    private $png_cdn = 'https://s.w.org/images/core/emoji/11/72x72/';
    private $svn_cdn = 'https://s.w.org/images/core/emoji/11/svg/';

will have to become something like

dirname( dirname( dirname( __FILE__ ) ) ) . '/src/wp-includes/core/emoji/11/..

(seeing wp-includes referenced like that in other files) ?

senlin commented 5 years ago

By the way, this adds no less than 8MB to the install, is that really necessary? Do business sites need emojis in core?

nylen commented 5 years ago

We need to make sure that we point the new files at a valid URL.

See also discussion on ClassicPress/ClassicPress#212 - I'm thinking we will want to serve the SVG emojis only, using a php file that returns the right image based on a query string argument.

This will be a pretty large PHP file, but at least it will compress down to a really small size in the build.

nylen commented 5 years ago

is that really necessary? Do business sites need emojis in core?

I agree with you that sites should be able to disable this easily, and I think this part is a better fit for moving to a core plugin in v2.

We could also look at adding (an) option(s) to disable specific emoji-related functionality, but I'd like to get some other people's opinions on that first.

senlin commented 5 years ago

I already removed the PNG files from the PR, how do you want to proceed with this?

nylen commented 5 years ago

We can do the work above in the current PR.

About the first option: The existing PR is still a good starting point and it would mean more work to close it.

About the third option: It's not ideal to be merging parts of unfinished features that leave "loose ends" or bugs in the code. Ideally we'll merge the changes here all at once.

senlin commented 5 years ago

Ok great, thanks for explaining that :)

Now the next questions are:

  1. how to merge the 2800+ SVG files into 1PHP file?
  2. how to write a query that pulls the correct SVG out of that new PHP file?

re 1) I have been looking around on how to merge SVG files but cannot seem to find anything relevant re 2) let's get 1 done first :)

nylen commented 5 years ago

https://github.com/Automattic/gridicons/blob/ea70d049fd9ac713ff3235e9c9f22b0c4bf94662/php/gridicons.php

Almost exactly this, except we need to return an SVG result to the browser based on the value of $_GET['char'] or similar.

nylen commented 5 years ago

Moved to beta3 milestone due to lack of time.

nylen commented 5 years ago

@invisnet has been working on this and he realized that there were some significant drawbacks with the approach I proposed above.

There is a much less complicated way to handle this issue for v1 - namely, host the emoji files on ClassicPress servers. Then it's just a matter of changing s.w.org to point at our server instead.

The code related to emojis will be moved to a plugin early in v2 - this is currently the top-rated ClassicPress petition:

2019-02-18t14 52 57-05 00

Since this is going away soon, let's not spend any more time re-working this code than we have to.

PR ClassicPress/ClassicPress#212 has been updated with this approach instead, I'm finishing a couple of final steps with the tests and then merging it.

ClassyBot commented 3 years ago

This issue has been mentioned on ClassicPress Forums. There might be relevant details there:

https://forums.classicpress.net/t/remove-emoji-support/3310/3