Show admin notice when indexing is discouraged #369

QWp6t · 2018-07-31T19:42:07Z

I haven't tested this yet. I'll leave that to someone else (@vpillinger?) because I'm lazy.

Closes #368

JulienMelissas · 2018-07-31T19:45:06Z

web/app/mu-plugins/disallow-indexing.php

@@ -3,12 +3,21 @@
 Plugin Name:  Disallow Indexing
 Plugin URI:   https://roots.io/bedrock/
 Description:  Disallow indexing of your site on non-production environments.
-Version:      1.0.0
+Version:      2.0.0
 Author:       Roots
 Author URI:   https://roots.io/
 License:      MIT License


Do we want to add the text domain for "roots"? 💋

this feature is becoming a beast

knowler · 2018-07-31T20:36:13Z

Tested and it works. 👍

Changed notice to a warning instead of an error + added text domain. Ready to be squashed and merged.

aaemnnosttv

One suggestion and one question, otherwise looks good!

aaemnnosttv · 2018-07-31T21:30:02Z

web/app/mu-plugins/disallow-indexing.php

+} else {
+    add_action('admin_notices', function () {
+        $message = __('Search engine indexing is currently discouraged.', 'roots');
+        echo sprintf('<div class="notice notice-warning"><p>%s</p></div>', $message);


Use printf() instead of echo sprintf()

why use either? why not just inline the var?

aaemnnosttv · 2018-07-31T21:34:50Z

web/app/mu-plugins/disallow-indexing.php

    add_action('pre_option_blog_public', '__return_zero');
+} else {


The else is not really necessary as there's no harm in hooking onto an admin hook for a non-admin request. I would personally omit it and save the extra braces + indentation; is this a WP/Roots style convention?

knowler · 2018-07-31T23:50:49Z

Maybe it could be helpful to append something about the current environment to the notice. It could help identify misteaks.

“Why?” would link to the plugin description in the repo, the site’s must-use plugin page, or somewhere else that’s more informative.

QWp6t · 2018-08-01T01:17:03Z

"Why?" could link to admin_url('plugins.php?s=disallow-indexing') or something similar imo.

* Remove conditionals * Change wording * Display current environment

knowler · 2018-08-01T04:45:10Z

Based on the feedback above I removed the conditional, used inline vars (thanks @austinpray), and made the message read a bit better.

Search engine indexing is discouraged [for/by/in/with] the current environment

could be read as a suggestion, when what we are actually doing is informing the user of what is happening.

Also, I decided against the "Why?" link and replaced it with Bedrock because:

mu-plugins don't have pages, so admin_url('plugins.php?s=disallow-indexing') doesn't work.
It felt redundant linking to a description that is not so much different than the message itself.
The plugin is a part of the functionality that Bedrock provides.
There is less to add code-wise. 😛

Unless anyone has any issues with this or wants to refine this more, we are ready to go (again).

QWp6t

I'm a fan of the "Why?" link that you had. Anyone else wanna weigh in on that?

QWp6t · 2018-08-01T13:30:08Z

web/app/mu-plugins/disallow-indexing.php

 }
+
+add_action('pre_option_blog_public', '__return_zero');


I'm pretty sure there needs to be !is_admin() check around this to prevent WordPress admin panel from always showing the option as enabled.

QWp6t · 2018-08-01T13:35:18Z

web/app/mu-plugins/disallow-indexing.php

+
+add_action('admin_notices', function () {
+    $message = __('Search engine indexing has been discouraged because the current environment is', 'roots');
+    echo "<div class='notice notice-warning'><p><strong>Bedrock:</strong> {$message} <code>".WP_ENV."</code>.</p></div>";


$message = sprintf(__('%1$s Search engine indexing has been discouraged because the current environment is %2$s.', 'roots'), '<strong>Bedrock:</strong>', '<code>'.WP_ENV.'</code>'); echo "<div class='notice notice-warning'><p>{$message}</p></div>";

JulienMelissas · 2018-08-01T13:52:58Z

I'll weigh in @QWp6t... I liked the "Why?" - especially if it just leads to some documentation.

austinpray

So in light of #380

Why not make this behavior explicit in the configuration? I added my comments inline.

austinpray · 2018-08-10T15:03:31Z

web/app/mu-plugins/disallow-indexing.php

 License:      MIT License
 */

-if (defined('WP_ENV') && WP_ENV !== 'production' && !is_admin()) {
-    add_action('pre_option_blog_public', '__return_zero');
+if (!defined('WP_ENV') || WP_ENV === 'production') {


What if we just did Config::define('DISALLOW_INDEXING', WP_ENV !== 'production'); in config/application.php

and then

if (!defined('DISALLOW_INDEXING') || DISALLOW_INDEXING !== true) { }

It makes the code read a bunch better:

Currently:

If we are on prod don't disallow indexing

Proposed:

If we aren't on prod disallow indexing

Additionally this makes the functionality

super easy to trace via config/application.php

does not rely on magic string 'production'

ability to be overridden per environment with new config system

austinpray · 2019-06-09T17:50:48Z

Since we are definitely doing extracting out the plugin autoloader (#299) we should probably extract this guy out as well and then implement my suggestions here #369 (comment)

retlehs · 2020-05-20T01:34:06Z

picking this up again - i've made https://github.com/roots/bedrock-disallow-indexing and pushed some initial code with @austinpray's suggestion

i also added a filter to prevent the notice from showing

see #521

Show admin notice when indexing is discouraged

3649719

JulienMelissas reviewed Jul 31, 2018

View reviewed changes

Add text domain + change notice to warning

0eaa249

QWp6t requested a review from swalkinshaw July 31, 2018 20:41

swalkinshaw approved these changes Jul 31, 2018

View reviewed changes

aaemnnosttv suggested changes Jul 31, 2018

View reviewed changes

knowler added 2 commits July 31, 2018 21:45

Improve admin notice

66ea2ee

* Remove conditionals * Change wording * Display current environment

Don't use env()

5c4b26a

knowler mentioned this pull request Aug 1, 2018

Clarify behavior of Disallow Indexing plugin roots/docs#153

Merged

QWp6t commented Aug 1, 2018

View reviewed changes

l a z y b o i

7e2fb81

austinpray suggested changes Aug 10, 2018

View reviewed changes

retlehs mentioned this pull request May 20, 2020

Move disallow indexing mu-plugin to package #521

Merged

retlehs closed this May 20, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Show admin notice when indexing is discouraged #369

Show admin notice when indexing is discouraged #369

QWp6t commented Jul 31, 2018 •

edited

Loading

JulienMelissas Jul 31, 2018

kalenjohnson Jul 31, 2018

knowler commented Jul 31, 2018

aaemnnosttv left a comment

aaemnnosttv Jul 31, 2018

austinpray Jul 31, 2018

aaemnnosttv Jul 31, 2018

austinpray Jul 31, 2018

knowler commented Jul 31, 2018 •

edited

Loading

QWp6t commented Aug 1, 2018

knowler commented Aug 1, 2018

QWp6t left a comment

QWp6t Aug 1, 2018

QWp6t Aug 1, 2018

JulienMelissas commented Aug 1, 2018

austinpray left a comment

austinpray Aug 10, 2018

austinpray commented Jun 9, 2019

retlehs commented May 20, 2020

		add_action('pre_option_blog_public', '__return_zero');
		} else {

Show admin notice when indexing is discouraged #369

Show admin notice when indexing is discouraged #369

Conversation

QWp6t commented Jul 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knowler commented Jul 31, 2018

aaemnnosttv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

knowler commented Jul 31, 2018 • edited Loading

QWp6t commented Aug 1, 2018

knowler commented Aug 1, 2018

QWp6t left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JulienMelissas commented Aug 1, 2018

austinpray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austinpray commented Jun 9, 2019

retlehs commented May 20, 2020

QWp6t commented Jul 31, 2018 •

edited

Loading

knowler commented Jul 31, 2018 •

edited

Loading