Make WordPress Core

Opened 9 years ago

Closed 6 years ago

Last modified 6 years ago

#31779 closed enhancement (fixed)

Warn users before using a built-in file editor for the first time

Reported by: helen's profile helen Owned by: helen's profile helen
Milestone: 4.9 Priority: high
Severity: normal Version:
Component: Themes Keywords: has-patch ui-feedback commit
Focuses: ui, administration Cc:

Description

As a person who began tinkering with her WordPress site via the theme and plugin file editors, I am not keen to get rid of them entirely. However, I think we could also do some small amount of education for our users. To that end, I suggest that on a per-user basis, a prompt be shown warning them of the dangers and suggesting alternative methods / linking to more documentation, for both the theme and plugin file editors. If they choose to continue, that prompt should not be shown again for that given editor.

Attachments (14)

first-time-editor-notices.diff (2.3 KB) - added by toddnestor 9 years ago.
Patch that adds the first time user notices for both the plugin and theme editors.
Screen Shot 2016-08-24 at 9.34.25 PM.png (73.8 KB) - added by helen 8 years ago.
31779-33.diff (52.0 KB) - added by aryamaaru 7 years ago.
patch for #31779
Edit Reminder.png (17.0 KB) - added by lukecavanagh 7 years ago.
Edit Reminder Example
31779.diff (12.2 KB) - added by ZaneMatthew 7 years ago.
example-warning.png (477.3 KB) - added by melchoyce 7 years ago.
tools-example.png (53.4 KB) - added by cliffseal 7 years ago.
WF - Interstitial.png (219.9 KB) - added by melchoyce 7 years ago.
WF - CSS Warning.png (237.6 KB) - added by melchoyce 7 years ago.
31779.2.diff (2.2 KB) - added by helen 6 years ago.
31779.3.diff (4.6 KB) - added by helen 6 years ago.
JS nonsense start for sharing
31779.4.diff (7.9 KB) - added by adamsilverstein 6 years ago.
31779.5.diff (7.7 KB) - added by helen 6 years ago.
31779.6.diff (6.2 KB) - added by westonruter 6 years ago.
Re-use pointer dismissal API and add link to Custom CSS section in Customizer. Δ https://github.com/xwp/wordpress-develop/compare/92e0b6a...trac-31779

Download all attachments as: .zip

Change History (100)

#1 @helen
9 years ago

A related thought, would probably warrant its own ticket were we to do it: I don't want to add "make a child theme" buttons everywhere in core (plugins do that just fine), but it could be a nice touch to prompt to create one if somebody is headed into the theme file editor.

#2 @MikeHansenMe
9 years ago

We have actually implemented this on a few of our onboarding tests and had zero complaints.

#3 follow-up: @magicroundabout
9 years ago

Hi guys,

I don't know if this is appropriate here, but I came to trac to see if there was a discussion about the idea of making the plugin and theme editors off by default and I couldn't find one, but I did find this.

My opinion is that these shouldn't be removed entirely, but they should be disabled - in the config - by default. My reasoning for this is security. I've probably worked on about 100 WordPress sites in the last four years and had maybe 5 or 6 sites hacked. Every time analysis of the logs has shown that after an initial entry (either by brute force or by some unknown method) the hacker bots proceeded to inject code using the theme editors.

I now add

define('DISALLOW_FILE_EDIT', true);

to wp-config.php as a matter of course. It's like a nervous tick: install WordPress, disable editors.

YES, I am totally aware that this is not a security risk if other security is present and correct: brute force protection, strong passwords, etc. But I should add that most of these sites that I'm rescuing are not sites that I developed - they are places where I'm bailing out another developer or an un-knowing user.

Is there a good reason why this feature, which seems to me to be a huge security issue in WordPress, is not off by default? I would suggest that this config option should be ALLOW_FILE_EDIT and it should be false by default. I would be quite happy for this to be an option during the WordPress install (though I doubt that would be a good idea as the user probably wouldn't understand the option in many cases). It would need to be backwards-compatible somehow to prevent an update breaking users' installs.

I'm happy to spin off another ticket to discuss. But this seemed an appropriate place to raise it. I hope you don't mind.

#4 in reply to: ↑ 3 @helen
9 years ago

Replying to magicroundabout:

If somebody has the ability to edit their wp-config.php, they are already a little past the point of needing/using an in-browser editor. Per the ticket description:

As a person who began tinkering with her WordPress site via the theme and plugin file editors, I am not keen to get rid of them entirely.

"Entirely" could also be "by default". This has been discussed in other venues a number of times - I don't see us disabling/removing them by default.

#5 follow-up: @magicroundabout
9 years ago

Thanks Helen, I'm sure it has been discussed before, I just can't find anywhere in trac or Google. Google is mostly people like me telling others how to disable the editors. I was hoping to find a rationale for keeping these things active. Can someone sum up why it is felt that the editors shouldn't be removed? Are there some stats showing usage? Is there research showing that users need or want these things and that if we take them away they will complain?

The amount of pain that these features cause me, as a developer, and my clients and end users FAR outweighs any benefit that we get from them, and even this ticket suggests that they should be used with caution. I'd turn your argument about ability/access on its head: if someone wants to be editing themes and plugins then they should probably be using something more sophisticated than an in-browser editor.

Thanks

#6 in reply to: ↑ 5 @helen
9 years ago

Replying to magicroundabout:

I'd turn your argument about ability/access on its head: if someone wants to be editing themes and plugins then they should probably be using something more sophisticated than an in-browser editor.

Yes, users *should* be using something else. However, they may not know any better. I was one of those users: I knew what FTP was, but only in the framework of getting MP3s from Audiogalaxy sources. I did not know what it had to do with making my blog look the way I wanted it to - if WordPress (like Blogger) hadn't provided in-browser methods for tinkering with my sites, I would never have gotten into WP development, full stop. I'm not here for the nostalgia, though - I'm saying this is a good place for a little bit of "are you sure about this" as well as education, not wholesale preventing users from learning that this world of editing the web even exists.

#7 @magicroundabout
9 years ago

I'm all up for education, but I'm still not hearing a convincing argument for keeping these features. There are better ways of learning the web these days - CodeAcademy, etc. My opinion would be that editing a PHP file where an incorrectly-placed piece of syntax white-screens your site so that you need to learn to use FTP anyway isn't a great way to learn to code. It's actually pretty frustrating.

Blogger let you use a templating language with tags, from what I remember. So these things aren't equivalent. The problem with the WordPress editor is that it lets you edit and create PHP.

A learning to code feature would be better on WordPress.com, but WordPress.com doesn't let me edit PHP files does it? I've never gone premium, but from what I can tell it only lets users edit CSS, which I think is an awesome idea - we should allow that, and there are good plugins (including JetPack) that let us do it. I'd love to see a built-in CSS editor.

But for WordPress.org? I'd say that by the time you've bought a domain, got a hosting package, logged into a cPanel or whatever and installed WordPress, you may as well have learned how to use FTP as well. Given that if you're editing code you'll probably end up white-screening anyway and needing FTP to recover, is it not better to teach people the FTP/control panel skills up front so they're not learning it in "ARGH MY BLOG IS DOWN" crisis mode? You could even teach them FTP by instructing them in how to add the "ALLOW_FILE_EDITORS" constant to wp-config.php?

In fact, the installation instructions for WordPress say:

Before you begin the install, there are a few things you need to have and do. These are:
Access to your web server (via shell or FTP)
A text editor
An FTP Client
Your web browser of choice

And if you're not doing it that way then you're using an installer in a hosting control panel where you probably have a file manager anyway, which is going to be a better experience than the theme/plugin editors.

I get what you're saying, I really do. But I just feel like these editors are a hinderance, even to people who want to learn to code and tinker with their site.

I'd definitely not say "do away with all ability to customise your site", I'm sure there are creative solutions to allow people that facility and I'd love to see them discussed. Could we add a PHP linter? Or some means of validating/sanitizing code. But the ability to add PHP in an un-sanitized manner is dangerous and I'd like to see it removed.

#8 @georgestephanis
9 years ago

I know there's previously been discussion of adding 'reauth' sections in the admin -- things you need to enter your password again to access. This would be as good of a candidate as any I've seen, just due to the potential extra danger of a session stealer injecting naughty php code via wp.

#9 @brocheafoin
9 years ago

I'm with @magicroundabout. I have just created ticket #31952 on the subject. My impression is that the file editor is a throwback to the days of the hacks.php file, when there where no widgets, etc. I'm pretty sure the most heavy users of this feature are hacking bots.

#10 @Otto42
9 years ago

#31952 was marked as a duplicate.

#11 @magicroundabout
9 years ago

I should add that @nacin was involved in an exchange on Twitter on this topic.

His stance was that the plugin and theme upload capability posed as much of a threat as the editors. I said that that may be true, but the uploaders aren't (in my experience, at least) being exploited. To which his response was that it's an arms race. Which I take to mean that if we lock down the editors then they'll head to the uploaders for their exploits.

(I hope this is an accurate interpretation of that conversation - apologies to Andrew if I've misrepresented his view)

I don't really have a response to the arms war comment. I think he's right. But I also think @brocheafoin is right in his new-but-closed ticket: the editors aren't actually much help anyway and end up getting users into trouble.

I think what I'd like to see here is a more informed and creative conversation about both the editors and the uploaders. If these are frequently exploited (or exploitable) functions from a security point of view, and cause users pain as much as they help them, then let's find some better solutions, as I don't feel like education is enough. I think (but I have no proof) that if you put a "This thing is dangerous and here's why" warning on the page, people will ignore it, as much as they ignore terms and conditions.

Some of my previous questions stand:

  • what are the usage stats for the editors?
  • is there any evidence that users will miss them?
  • could we come up with some way of ensuring that code modified through the editors is not malicious or broken? e.g. If someone proposed including a linting process, or even syntax checking, before saving a PHP file, then that might help.

But also I wonder if there are better ways to secure both the editor and uploader functions, or at the very least to monitor activity through them. Has use of a captcha (or honeypot, or similar) ever been discussed? Or some kind of re-verification? Or a log of changes so that hacks can be detected? I'm not a security expert, so I don't know what the modern-day options are here for preventing unwanted changes.

There seems to be acceptance that there is an issue here. The proposed solution is education/notification. I'd like to see some more options. Happy to be referred to a previous debate/discussion somewhere if this has already done the rounds.

Thanks

#12 @iseulde
9 years ago

  • Component changed from Editor to Themes

#13 @helen
9 years ago

  • Keywords needs-patch good-first-bug added

I would still like to see an interstitial prompt, so if somebody would like to make a patch, we can start looking at getting it in. I think we would also need to review or possibly write a good reference page in the Codex. Do note that this is a single targeted enhancement to an existing feature. There are a lot of other great ideas wrapped up in comments above that can (and should!) be their own distinct tickets if they are not already, like sandboxing/error checking and such.

I agree that there are better learning materials out there - what I see is that the person needs to know to be looking for those in the first place. We can wish all we want that people come into WordPress with some amount of web development knowledge, but that's just not the case, especially with one-click installers (me, even now!). Sure, there are file managers somewhere in cPanel if you can find them, but that still assumes you know where to browse to find the file you're looking for, and if you're not familiar with WP, do you know where to look? Do you know to go into public_html and then wp_content and then themes and then twentyfifteen and then maybe which file you want? I know I wouldn't have.

I say this frequently in talks: when you're running at the scale WordPress is running on the internet, you have this incredible opportunity to show people that web development is a thing that exists. Not teaching how to do it, not showing off with our code (ha, ha) - just that it exists at all, and maybe how to learn about it. Intrepid learners can take it from there. It sounds so basic in our world, seeing as we're here on Trac, but you can't have options if you don't know what they are. I would like to see users helped along better and saved from hurting themselves as much as possible, so let's make this experience good enough to be safe but not so good it prevents users from ever using something else. :)

#14 @toddnestor
9 years ago

I would like to make a patch for this as my first patch I make for the Wordpress Core. I am going to work on it right now.

@toddnestor
9 years ago

Patch that adds the first time user notices for both the plugin and theme editors.

#15 @toddnestor
9 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

I added a patch to the attached files, I couldn't really find a good article about how to submit a patch (https://make.wordpress.org/core/handbook/tutorials/trac/submitting-a-patch/ only seems to tell you how to make one), so I am assuming that is how I do it.

#16 follow-up: @RSPublishing
9 years ago

Interesting!

That given 'editor' would only be either Super admin (mu) or admin (single), correct.?

While new to this, in my head:

1 > given a prompt urging to backup folders/files before continuing
2 > given another prompt warning him of potential dangers/alternatives
3 > given option to proceed (enter editor) / cancel (redirect to dashboard)

Wouldn't mind playing around with this.

#17 in reply to: ↑ 16 @toddnestor
9 years ago

I actually wrote a patch for this that you can see in the attachments. However I don't know how to submit the ticket or get it over to testing.

Replying to RSPublishing:

Interesting!

That given 'editor' would only be either Super admin (mu) or admin (single), correct.?

While new to this, in my head:

1 > given a prompt urging to backup folders/files before continuing
2 > given another prompt warning him of potential dangers/alternatives
3 > given option to proceed (enter editor) / cancel (redirect to dashboard)

Wouldn't mind playing around with this.

#18 @johnbillion
9 years ago

  • Owner set to helen
  • Status changed from new to reviewing

#19 @jb510
8 years ago

+1 on this ticket, but +100 on removing the editor from core all together, I've long felt it should be a plugin and prompted for installation similar to the WordPress Importer along with upgrading it to provide basic syntax highlighting and checking.

Still, love the idea requiring reauth for using it and a warning prompt. Following.

#20 @celloexpressions
8 years ago

Related: #35395. Seeks to provide a better alternative to the code editors for, well introducing users to code, particularly for those like me (and also helen) who started out be playing with stuff in the theme and plugin editors. Not directly tied to the future of the editors but would certainly offer some direction. That ticket is still awaiting constructive feedback from the security team on how we could make it work/what's required; any assistance would be greatly appreciated.

Also +1 on this ticket.

#21 @Presskopp
8 years ago

All discussion aside, I don't really like the phrasing

You are editing the actual theme files. Make sure you know what you're doing!

From a user's perspective:
"What am I doing? I don't care, I want to edit something, WP nags me with a dismissable "Are you sure"-crap - if I wasn't sure, I wouldn't have gone to editor, stupid WP-developers!" ;-)

So I propose something more like:
Please be aware by editing a file this way you can easily do damage to your site - better keep it backed up

I changed this sentence while typing 4-5 times, because my mind didn't stop throwing alternatives. So it's only one example.

I'm not a so called security-expert, but +1 for https://core.trac.wordpress.org/ticket/31779#comment:19

#22 @Presskopp
8 years ago

Just found this in the translation files (wp-admin/theme-editor.php:32):

<strong>Advice:</strong> think very carefully about your site crashing if you are live-editing the theme currently in use.

There is a connection.

#23 follow-up: @helen
8 years ago

Screen Shot 2016-08-24 at 9.34.25 PM.png is a quick mockup of what I was imagining, which is an interstitial rather than a notice. Clicking the button would take you to the editor as it currently exists and the interstitial for editing a theme would no longer show for that specific user. Plugin editor should get the same treatment with its own separate user meta.

The important things here are:

  • The right language in all of the messaging, including letting the user know that they won't see this message again should they continue on to the editor. My mockup is by no means a hard suggestion, although I think it's not so bad.
  • A resource (on .org somewhere? Codex?) that talks about custom CSS, child theming, FTP, and maybe even with a link off to something about version control. This would be the "learn more" link.
  • A parallel resource for plugins.
  • Visually emphasizing that this is very important to read, maintaining hierarchy and still looking good.

Each of these items can be worked on separately, and if one wants to take a stab at a patch for the functionality of it, that is also a good start (although there should be agreement that this is a good approach).

Reauth may be a nice add as well, but let's do this part first.

#24 follow-up: @brocheafoin
8 years ago

Does this mean this task is blessed for 4.7?

Can we work on the wording right here or do we have to submit patches?

#25 in reply to: ↑ 23 ; follow-up: @boogah
8 years ago

Replying to helen:

Clicking the button would take you to the editor as it currently exists and the interstitial for editing a theme would no longer show for that specific user.

I am incredibly excited about this! It'd be nice to have the ability to not hide the interstitial after it has been dismissed though. Via a filter, perhaps?

#26 in reply to: ↑ 24 @helen
8 years ago

  • Keywords needs-patch added; has-patch needs-testing removed

Replying to brocheafoin:

Does this mean this task is blessed for 4.7?

Can we work on the wording right here or do we have to submit patches?

I generally don't move existing tickets into a milestone without a patch, but so long as there's general agreement that this is a good move and patches are made, I imagine it would go into 4.7.

Language work can go on right here on Trac, it can be annoying to do via constant repatching.

Replying to boogah:

It'd be nice to have the ability to not hide the interstitial after it has been dismissed though. Via a filter, perhaps?

I guess there are two potential methods here, one that is kind of hacky but not adding new hooks, and the other new hooks. I suppose a new hook makes sense in particular for managed instances and hosts - curious if in those cases you'd want to be linking off to your own resource instead of .org.

This ticket was mentioned in Slack in #design by hugobaeta. View the logs.


8 years ago

#28 in reply to: ↑ 25 ; follow-up: @brocheafoin
8 years ago

Replying to boogah:

It'd be nice to have the ability to not hide the interstitial after it has been dismissed though. Via a filter, perhaps?

I don't see the use case for this. If you're that worried about your users that you want them to see the warning all the time, you should probably just DISALLOW_FILE_EDIT.

Design-wise, I think the interstitial should be as in-your-face as Chrome or Firefox' malware/phishing warning. It really _is_ the most dangerous part of the WordPress admin. One false move could turn your whole site, including the admin, into a white screen of death. So here's my wording suggestion for the theme editor:


Danger Ahead!

This theme editor is provided as a convenience, but it is not the recommended way to modify a theme.

One false move can render your whole site unusable. That means you won't even be able to come back here to undo what caused the error. You would need to restore a backup using FTP. If you don't know what FTP is, you probably should not be using this editor.

Furthermore, even if everything goes well, as soon as the theme is updated, you will lose all the changes you have made.

If you want to make small style changes, consider intalling Jetpack and using its Custom CSS module. It provides a better editor, keeps track of your changes, can not break your site and will keep your modifications when the theme is updated.

If the changes you want to make are more substantial, the recommended approach is to set up a local development environment and create your own child theme. This will isolate your modifications and make sure they are retained when the original theme is updated.

Primary button label: Back to safety
Secondary button label: Just leave me alone, I know what to do


Reference for the secondary button label: https://www.youtube.com/watch?v=nlbjnZbxEcI

Last edited 8 years ago by brocheafoin (previous) (diff)

#29 follow-up: @celloexpressions
8 years ago

I generally like the text in 28, although we should not point them to Jetpack for custom CSS (it isn't in the customizer). Hopefully #35395 will be ready for 4.7, or if not there are numerous other, better plugins to suggest.

#30 in reply to: ↑ 29 @brocheafoin
8 years ago

Replying to celloexpressions:

we should not point them to Jetpack for custom CSS (it isn't in the customizer).

I put Jetpack in there because it's the one I use for quick CSS mods. I honestly thought it was available in the Customizer since that's the way it works on WP.com.

Personally, I hate typing CSS in the Customizer's narrow column but to each their own. I really don't care what it is, but we need to link to something allowing custom CSS.

Hopefully #35395 will be ready for 4.7

I did not know Custom CSS was considered as a future part of core. If it's to happen, obviously we should just point there.

there are numerous other, better plugins to suggest.

I am all ears. Maybe it would be better if the notice suggested a few plugins so it doesn't appear to favour one in particular?

#31 @DrewAPicture
8 years ago

I know it's been discussed previously, but it's also worth reconsidering merging the Code Revisions plugin that was developed as a WordPress GSOC project a couple of years ago. Perhaps not so much the revisioning aspect, but more-so the breaking changes safeguards that prevent actively breaking a site when making edits via the file editors.

#32 in reply to: ↑ 28 @voldemortensen
8 years ago

Replying to brocheafoin:

Replying to boogah:

It'd be nice to have the ability to not hide the interstitial after it has been dismissed though. Via a filter, perhaps?

I don't see the use case for this. If you're that worried about your users that you want them to see the warning all the time, you should probably just DISALLOW_FILE_EDIT.

Design-wise, I think the interstitial should be as in-your-face as Chrome or Firefox' malware/phishing warning. It really _is_ the most dangerous part of the WordPress admin. One false move could turn your whole site, including the admin, into a white screen of death.

I think you just provided a use case. If it is indeed the most dangerous part of the admin, it should have reminders. Chrome and Firefox don't just show you a phishing warning for one site and then never warn you again. They do allow you to click through details, and then visit the site. But I don't think there's a way to permanently dismiss a phishing warning for a site (other than doing something like --disable-web-security in chromium or similar for other browsers).

I think it should show every time a file editor is loaded by default, with a filter to only show it once or disable it.

@aryamaaru
7 years ago

patch for #31779

#33 @ZaneMatthew
7 years ago

Is the patch still valid?

When applying the patch via

grunt patch:31779

I'm getting the following;

Running "patch:31779" (patch) task
? Please select a patch to apply 31779-33.diff (52.0 KB) - added by aryamaaru 4 months ago.
patching file wp-admin/themes.php
Hunk #1 FAILED at 158.
1 out of 1 hunk FAILED -- saving rejects to file wp-admin/themes.php.rej
patching file wp-admin/theme-editor.php
patching file wp-admin/js/theme.js
Hunk #1 FAILED at 83.
1 out of 1 hunk FAILED -- saving rejects to file wp-admin/js/theme.js.rej
can't find file to patch at input line 58
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|Index: wp-admin/js/theme.min.js
|===================================================================
|--- wp-admin/js/theme.min.js	(revision 38598)
|+++ wp-admin/js/theme.min.js	(working copy)
--------------------------
File to patch:
Skip this patch? [y] y
Skipping patch.
1 out of 1 hunk ignored

Done, without errors
Last edited 7 years ago by ZaneMatthew (previous) (diff)

#34 @ZaneMatthew
7 years ago

I was able to get the patch working, just a few things seemed odd. Currently working on adjusting it.

@lukecavanagh
7 years ago

Edit Reminder Example

@ZaneMatthew
7 years ago

#35 @ZaneMatthew
7 years ago

  • Keywords has-patch added; needs-patch removed

@lukecavanagh I just submitted a patch based the styles that are inline with core, see @helen mockups.


Aside from removing the "Editor" from core, I feel strongly that this is a good approach. Granted, there are helpful resources in that seldom-clicked-on-help-menu.

I've submitted a patch based on Helen's suggestion that does the following;

  • Given a users first time visiting the "Edit Themes" page a message is displayed. This message affords; a verbose or concise statement, resource URLs, and an action to be taken.
  • Once the user takes an action, their user meta is updated to no longer display this message.

A question for those following;

#36 follow-up: @lukecavanagh
7 years ago

@ZaneMatthew

I do not have a problem with keeping the editor in core, but a warning for end-users before editing plugins or theme files is a decent change.

It makes sense to show the warning to first time users on site before editing.

Last edited 7 years ago by lukecavanagh (previous) (diff)

#37 @ZaneMatthew
7 years ago

While working on another project I was greeted with a message that solves the issue we are discussing above.

Caution: You can break stuff here.

Its coarse, yet concise.

If the above patch has the desired work flow, we should focus next on content, including URLs to send visitors to for guidance. The next round of revisions on the patch should include URLs, and content (maybe the statement above ;)

#38 in reply to: ↑ 36 @ZaneMatthew
7 years ago

@lukecavanagh I agree as well, it is a "nice to have" feature/notice for first time users.

Replying to lukecavanagh:

@ZaneMatthew

I do not have a problem with keeping the editor in core, but a warning for end-users before editing plugins or theme files is a decent change.

It makes sense to show the warning to first time users on site before editing.

#39 @westonruter
7 years ago

  • Milestone changed from Awaiting Review to 4.9

This ticket was mentioned in Slack in #design by melchoyce. View the logs.


7 years ago

#41 @melchoyce
7 years ago

We'd like to resurrect this for 4.9. Some ideas:

If you're editing your theme:

Looks like you're trying to edit your theme! We recommend creating a [child theme](link to documentation) instead. This will let you make changes to your theme without accidentally breaking anything."

Except with better copy. 😅

We could consider making a "make child theme" button that generates a child theme with a functions.php and style.css.

Once someone makes a child theme, whether manually or using this button, we should link to the child theme documentation at the top of the screen.

If someone's specifically editing their style.css, we should consider showing an additional warning to directly them to Additional CSS:

Hey, did you know there's a safer way to edit your site's CSS? Check out [Additional CSS](link) in the Customizer.

If you're editing plugins:

Looks like you're trying to edit your plugin. Editing this code is highly discouraged; please proceed with caution and make sure to back up your plugin before making edits.

(Though I have to say, the brevity of "Caution: You can break stuff here" is appealing.)

#42 @melchoyce
7 years ago

#41075 was marked as a duplicate.

#43 @Ipstenu
7 years ago

Why not both?

Title: Caution: You can break stuff here.

Body: You appear to be making direct edits to your [theme|plugin]. Editing this code directly is dangerous as it can leave you unable to log back in to WordPress and undo changes. [We strongly recommend using a child theme for any edits to your theme.|You can edit CSS in WordPress's built in CSS editor.|Editing plugin code directly is highly discouraged.] Please proceed with caution and make sure to back up your site before making any changes.

We'd want to detect if they're already on a child theme, of course.

#44 @melchoyce
7 years ago

Tested out @Ipstenu's text for themes in example-warning.png.

This ticket was mentioned in Slack in #core by melchoyce. View the logs.


7 years ago

#46 follow-ups: @cliffseal
7 years ago

If it's not an option to remove or disable-by-default this editor, I think a combination of other simultaneous changes could support this effort:

  1. Move the Editor into the Tools menu, or into Tools > Available Tools. It fits better there anyway: it's not a visual editor for the Appearance, as other things are in the Appearance menu), but the other problem with Plugins > Editor is it's right beside one of the most clicked buttons in the UI (Add New). It's too easy to discover. You could also move the warning messages suggested above into a box on Available Tools. Instead of a warning, it becomes the context for launching the Editor. Plus, what else is going on in that page anyway. :)
  2. To do the above, combine the Theme and Plugin editors into one. Separate the edit dropdown with headings for Theme and Plugins.
  3. Make Editor access a setting in Settings > General. I saw the argument that having to set the constant doesn't make sense for the people using the Editor—this is a solution that still hides the danger by default but has a low barrier to enablement.
  4. IMO, no extra attention should be brought to this editor until it checks for PHP fatal errors before save. This is the number 1 warning I've had to give people for years, and the possibility of failure is too high with consequences that are too drastic (lock outs).

#47 @melchoyce
7 years ago

IMO, no extra attention should be brought to this editor until it checks for PHP fatal errors before save. This is the number 1 warning I've had to give people for years, and the possibility of failure is too high with consequences that are too drastic (lock outs).

We're also hoping to resurrect #12839!

#48 @melchoyce
7 years ago

Some copy suggestions from the magnificent @michelleweber:

Heading for all

Caution: You can break stuff here. There’s a better way!

Theme Template Files

You appear to be making direct edits to your theme in the WordPress Dashboard. We recommend that you don’t! Editing this code directly is dangerous, and can leave you unable to log back in to WordPress and undo changes. To edit your theme without breaking anything, use a child theme instead — [here’s how](link).

If you decide to go ahead with direct edits anyway, make sure to back all your site’s files up before making changes so you can restore a functional version is something goes wrong.

Theme CSS Files

You appear to be making direct edits to your theme in the WordPress Dashboard. We recommend that you don’t! Editing this code directly is dangerous, and can leave you unable to log back in to WordPress and undo changes. There’s no need to change your CSS here — [you can edit CSS in WordPress’s built in CSS editor](link).

If you decide to go ahead with direct edits anyway, make sure to back all your site’s files up before making changes so you can restore a functional version is something goes wrong.

The "and can leave you unable to log back in" isn't really true for purely CSS changes, so we might want to tweak that.

Plugins

You appear to be making direct edits to your plugin in the WordPress Dashboard. We recommend that you don’t! Editing plugins directly introduce incompatibilities that break your theme or other plugins, and can leave you unable to log back in to WordPress and undo changes.

If you absolutely have to edit this plugin, create copy with a new name and hang on to the original version, so you can re-enable a functional version if something goes wrong.

Or, if we want to be cheeky about it...

You appear to be making direct edits to your plugin in the WordPress Dashboard. We recommend that you don’t! Editing plugins directly introduce incompatibilities that break your theme or other plugins, and can leave you unable to log back in to WordPress and undo changes.

If you absolutely have to edit this plugin (please don’t), create a copy with a new name and hang on to the original version (but really, don’t do it), so you can re-enable a functional version if something goes wrong (seriously: don’t edit plugins).

#49 @cliffseal
7 years ago

tools-example.png is a mockup of my suggestion to use the Available Tools screen (ticket:31779#comment:46). Additional proper links should be added to child theme and CSS editor references, and pardon the spelling error. :)

Last edited 7 years ago by cliffseal (previous) (diff)

#50 @karmatosed
7 years ago

The first file loaded is the CSS file when you edit the theme, so having an encouraging 'have you thought about additional CSS' message, may help avoid discourage users.

Version 0, edited 7 years ago by karmatosed (next)

This ticket was mentioned in Slack in #design by helen. View the logs.


7 years ago

#52 @nic.bertino
7 years ago

Here's an idea:

http://nicbertino.com/wp-content/uploads/2017/08/File-Editor-Interstitial.png

I see this as an interstitial, and below the "I Understand" section, resources could be added for learning development in a way that is safer.

#53 in reply to: ↑ 46 @celloexpressions
7 years ago

Replying to cliffseal:

If it's not an option to remove or disable-by-default this editor, I think a combination of other simultaneous changes could support this effort:

  1. Move the Editor into the Tools menu, or into Tools > Available Tools. It fits better there anyway: it's not a visual editor for the Appearance, as other things are in the Appearance menu), but the other problem with Plugins > Editor is it's right beside one of the most clicked buttons in the UI (Add New). It's too easy to discover. You could also move the warning messages suggested above into a box on Available Tools. Instead of a warning, it becomes the context for launching the Editor. Plus, what else is going on in that page anyway. :)

+1

  1. To do the above, combine the Theme and Plugin editors into one. Separate the edit dropdown with headings for Theme and Plugins.

This makes a lot of sense. There are functionally few cases where the distinction between them needs to manifest in entirely separate editors. Much of the text, functionality, and warnings about changes being overwritten by updates apply to both plugins and themes. It also further promotes the use of additional CSS for visual adjustments, with the editor(s) being a place for more involved changes.

  1. Make Editor access a setting in Settings > General. I saw the argument that having to set the constant doesn't make sense for the people using the Editor—this is a solution that still hides the danger by default but has a low barrier to enablement.

In terms of discoverability for the people that benefit most from this functionality, it's probably better not to introduce a user-facing option for this.

#54 in reply to: ↑ 46 @melchoyce
7 years ago

Replying to cliffseal:

  1. Move the Editor into the Tools menu, or into Tools > Available Tools. It fits better there anyway: it's not a visual editor for the Appearance, as other things are in the Appearance menu), but the other problem with Plugins > Editor is it's right beside one of the most clicked buttons in the UI (Add New). It's too easy to discover. You could also move the warning messages suggested above into a box on Available Tools. Instead of a warning, it becomes the context for launching the Editor. Plus, what else is going on in that page anyway. :)
  2. To do the above, combine the Theme and Plugin editors into one. Separate the edit dropdown with headings for Theme and Plugins.

Let's split this into another ticket (if one doesn't exist already), since it's worth considering on its own, independent of whatever we do here.

FWIW, I mocked up a draft for what a post-Gutenberg code editor could look like in core. I think this design could work under tools, and includes a switcher between Themes/Plugins: https://github.com/WordPress/better-code-editing/issues/41

#55 follow-ups: @melchoyce
7 years ago

Okay... here's what I'm thinking :)

When you first visit either theme-editor.php or plugin-editor.php, you see - Interstitial.png, which warns you about the code editors and makes you write "I understand" to proceed. Once you do this, your user never sees it again.

If we want to present additional warnings, we can frame them as "tips." See - CSS Warning.png for an example. These tips will be dismissible notifications that only appear once. Once you dismiss them, they're gone.

Does this seem reasonable?

#56 in reply to: ↑ 55 @boogah
7 years ago

Replying to melchoyce:

Okay... here's what I'm thinking :)

When you first visit either theme-editor.php or plugin-editor.php, you see - Interstitial.png, which warns you about the code editors and makes you write "I understand" to proceed. Once you do this, your user never sees it again.

If we want to present additional warnings, we can frame them as "tips." See - CSS Warning.png for an example. These tips will be dismissible notifications that only appear once. Once you dismiss them, they're gone.

Does this seem reasonable?

I really like this, Mel. Great job! The "I understand" prompt is a good way to make sure that people don't blindly stab at the "Proceed" button to bypass the warning.

I think doing this once for theme-editor.php and once for plugin-editor.php would be great though as the plugin prompt should (probably) warn that their changes may be overwritten by a future update to the plugin and the theme prompt should push folks to the concept of child themes.

#57 @melchoyce
7 years ago

I think doing this once for theme-editor.php and once for plugin-editor.php would be great though as the plugin prompt should (probably) warn that their changes may be overwritten by a future update to the plugin and the theme prompt should push folks to the concept of child themes.

True, plugins and themes have different messages, so we should probably show both the first time you go to either editor.

#58 follow-up: @cliffseal
7 years ago

I think this is a good compromise, @melchoyce! Want me to go ahead and create the ticket you mentioned in ticket:31779#comment:54?

#59 in reply to: ↑ 58 ; follow-up: @melchoyce
7 years ago

Replying to cliffseal:

I think this is a good compromise, @melchoyce! Want me to go ahead and create the ticket you mentioned in ticket:31779#comment:54?

Please do! Thanks :)

#60 in reply to: ↑ 59 @cliffseal
7 years ago

Replying to melchoyce:

Replying to cliffseal:

I think this is a good compromise, @melchoyce! Want me to go ahead and create the ticket you mentioned in ticket:31779#comment:54?

Please do! Thanks :)

Done! #41804

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


7 years ago

#62 in reply to: ↑ 55 @westonruter
7 years ago

Replying to melchoyce:

If we want to present additional warnings, we can frame them as "tips." See - CSS Warning.png for an example. These tips will be dismissible notifications that only appear once. Once you dismiss them, they're gone.

I don't think that this should be dismissible. Or rather, instead of a warning it could be just a notice. When editing style.css in the theme editor, I think there should be a persistent link to open the Additional CSS section in Customizer, as noted in a Better Code Editing issue. This is similar to how there a persistent admin notice if you go to the legacy custom header admin screen:

https://user-images.githubusercontent.com/134745/28848417-b5c9de48-76c7-11e7-85b2-53bbda7b1e7e.png

Or it could be similar to how there is a “Edit with Live Preview” button on the widgets admin screen.

This ticket was mentioned in Slack in #core-customize by westonruter. View the logs.


7 years ago

#64 @westonruter
7 years ago

  • Priority changed from normal to high

Bumping priority to high for visibility and alignment with 4.9 goals, and given proximity to beta 1 deadline.

#65 @melchoyce
7 years ago

  • Keywords ui-feedback added

This ticket was mentioned in Slack in #design by karmatosed. View the logs.


7 years ago

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

#68 @jbpaul17
6 years ago

  • Keywords needs-refresh added

#69 @helen
6 years ago

I am looking at this today in hopes of finishing up for beta.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

This ticket was mentioned in Slack in #core by westonruter. View the logs.


6 years ago

@helen
6 years ago

@helen
6 years ago

JS nonsense start for sharing

#72 @adamsilverstein
6 years ago

31779.3.diff is a great start, I built on that in 31779.4.diff and added:

  • an ajax callback to handle the close action
  • a nonce to make the callback secure
  • a localized variable set to 'theme' or 'plugin' to save the correct user metta settings (its separate for each screen)

After these changes, once you dismiss the modal once it doesn't show up again on that screen. The modal text still needs adjusting, it says plugin on the theme screen for example. Also, I haven't tested the modal on mobal/various screen sizes.

@helen
6 years ago

#73 @helen
6 years ago

31779.5.diff adds focus handling, changes the theme warning to better match Michelle's text, and does a little bit of clean up. I think it's reasonable for a first commit, knowing that we may need to tweak focus handling, strings (including a link to custom CSS in the customizer), and JS style.

#74 @melchoyce
6 years ago

Tested, seems to work okay. I'm cool with us committing now and improving in beta. Have a couple things I'll note down for later.

#75 @melchoyce
6 years ago

  • Keywords commit added; good-first-bug needs-refresh removed

@westonruter
6 years ago

Re-use pointer dismissal API and add link to Custom CSS section in Customizer. Δ https://github.com/xwp/wordpress-develop/compare/92e0b6a...trac-31779

#76 @helen
6 years ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 41774:

File Editors: Introduce an interstitial warning upon first visit.

This is an effort to provide a speed bump before heading into something potentially destructive and some education for users on better alternatives, even as we make the file editors safer to use. Each user, including existing users, will be shown a one-time dismissible modal warning on their first visit to each of the theme and plugin file editors.

Copy tweaks to come.

props michelleweber, Ipstenu, melchoyce, adamsilverstein, westonruter, toddnestor, aryamaaru, ZaneMatthew, cliffseal, helen.
fixes #31779.

#77 @helen
6 years ago

Opened a new ticket for copyediting: #42100

#78 @westonruter
6 years ago

In 41775:

File Editors: Fix JSHint error after [41774].

See #31779.

#79 @helen
6 years ago

In 41859:

File editor warning: Add a "Go back" button.

see #42100, #31779.

#80 @psn
6 years ago

Hi, just wonder if the code define('DISALLOW_FILE_EDIT', true); which we use in wp-config.php file has been changed or disabled in WP core as the file editor for both plugins and theme is available and not hidden in dashboard?

Per

#81 follow-ups: @johnbillion
6 years ago

@psn Can you double check your config? I've tested the constant and it's still working as expected here. Make sure the definition is in the correct config file, make sure there's no typo, etc.

#82 in reply to: ↑ 81 @psn
6 years ago

Replying to johnbillion:

@psn Can you double check your config? I've tested the constant and it's still working as expected here. Make sure the definition is in the correct config file, make sure there's no typo, etc.

Have done that 4 times now but as I have checked my 2 other WP sites I can see its working for them so it must be something that has been corrupted for this site. I will try to manually re-install all wp-admin and wp-includes files from a fresh copy I have downloaded from wordpress.org. I hope that will help.

#83 in reply to: ↑ 81 @psn
6 years ago

Replying to johnbillion:

@psn Can you double check your config? I've tested the constant and it's still working as expected here. Make sure the definition is in the correct config file, make sure there's no typo, etc.

Hi John, re-install didn't help either so now I'm lost what next to do?

#84 @lukefiretoss
6 years ago

@psn

define( 'DISALLOW_FILE_EDIT', true );

Just checked on local dev running WP 4.9-beta4-42028 that constant is still working as expected.

https://codex.wordpress.org/Editing_wp-config.php#Disable_the_Plugin_and_Theme_Editor

#85 @johnbillion
6 years ago

I'd recommend that you enabled WP_DEBUG and see if there are any errors on your site about redeclaring that constant. It may be defined as false earlier on in the code.

Failing that, your best bet is to post on the wordpress.org/support forums and hopefully a volunteer will try to help you out.

#86 @ZaneMatthew
6 years ago

Great to see movement on this, and the communities desire to make WordPress' UI/UX exceed the experiencers expectations.

Note: See TracTickets for help on using tickets.