Enable revisions by default

I agree that this feature request is a good addition to the standard installation profile.

I've validated that the patch works as intended, enabling revisions by default for basic page and article content types.

Still to do:

• Make revisions on default for any new content type

@timmillwood Please re-supply patch using the 'Patchname suggestion' format (found under the 'Files' section on this page), so that your patch gets the automated tests run against it. (ie: enable_revisions_by-2490136-#.patch -- where # is the next available comment number.)

And, then, update the 'Status' to 'Needs review'. Thanks!

@timmillwood I like this issue in principle. If you think this could be considered in the beta phase, you will need a beta evaluation (add it with dreditor). Also, an issue summary update where a case is made for this change (use the usual template) is probably a good idea, and in addition a draft change record.

@timmillwood The change records are a statement of fact, so consider re-titling the change record "Node Revisions are Enabled by Default" or something similar.

Thanks!, Do we need to add new_revision key value TRUE in OR we will do that in another follow up ?

1. core/modules/book/config/install/node.type.book.yml
2. core/modules/node/tests/modules/node_test_config/config/install/node.type.default.yml

Seems also in the config_tests as well.

I think this is a great patch. We have a good revision system in core, let's use it!
Patch is very simple and it all looks good. There's also a change record in place already.

This came up in usability testing, albeit indirectly. Drupal caused a couple of participants to be so scared of breaking anything that they were unable to complete the tasks. This option would add some robustness.

So I would think enabling this by default in standard is totally cool, but I was less sure about enabling it it by default in the node type as we do here:

+++ b/core/modules/node/src/Entity/NodeType.php
@@ -88,7 +88,7 @@ class NodeType extends ConfigEntityBundleBase implements NodeTypeInterface {
-  protected $new_revision = FALSE;
+  protected $new_revision = TRUE;

i discussed this concern with @Berdir and @plach and asked them how we could retain the FALSE default but still make new node types in standard default to TRUE. @Berdir said that the only way that would be possible would be with standard altering the node entity form, which is probably worse than just changing the default. So we agreed to keep the current form of the patch.

Also, this is more of a task than a feature request. I can see the case for making this change during the beta for the usability impact as the disruption is low. We would not need an update function because existing sites and exported configuration would retain whatever value for the property was configured for that particular node type. However, there could be some disruption in that enabling this by default makes any existing bugs with node revisioning much more prominent. We've fixed some of the worst ones, but we should evaluate that before we commit to this change. So I think we need a beta evaluation update here that includes what new major bugs if any the site builder would now encounter out of the box. (Edit: meant a beta evaluation update.)

My one other concern was about scalability. Node revisioning being enabled results in much more being stored in the database, and I'd always assumed that was why revisions were not enabled by default. I talked to @plach and @Berdir about that too and they pointed out that a large site would need to be able to support this anyway since it's still a common configuration. I asked about the other end of the spectrum -- a small site with a low limit for the allowed size of the database -- but we decided that's not a concern either since those sites would also not be likely to have tons of updates being made to their content.

It also impacts the initial product experience when Drupal is installed in general. Based on #21 it sounds like @webchick is in favor of this change as a product manager.

TLDR of #22 is: check for outstanding major bugs impacting node revisions, mention them here, decide how bad it is, and if it isn't too bad set back to RTBC. :)

(Forgot to tag yesterday.)

I've also reviewed the issues timmillwood found in #25, and don't think any of them are severe enough that they should block enabling this by default.

Most are either performance-related or UI-related, and none result in data loss (they'd be critical in that case I suppose).

If anything, focusing more attention on the remaining revision-related issues by enabling this by default may provide enough attention to push fixes through.

Removing the beta phase tag since one has been added to the summary.

I'd vote +1 for RTBC at this point.

So here are the issues I'm most concerned about:

• #2261669: Slow query in NodeRevisionAccessCheck
• #2458223: Duplicated field handlers in field UI for some base table fields

Maybe we should do some profiling?

And then:

• #2465907: Node revision UI reverts multiple languages when only one language should be reverted
• #2362435: When viewing a revision, the Quick Edit, Edit, and Delete contextual link operations are available, but should not be
• #1863898: Add test coverage for Views revision link handlers

In the case of two of these, the fact that revisions are an opt-in feature was part of the reasoning for why they should be major rather than critical. Without spending too much time on it, I think I'd still consider all three of them major after this change, but they do have a greater impact.

I'll check with @catch and @webchick for feedback on this.

Just a quick followup, based on discussion with @catch, I'm planning to commit this after checking with @webchick. Thanks @timmillwood and @jhedstrom!

@webchick said that she wanted to think more about this and whether it would make more sense now or in 8.1.x, so assigning for that. Sorry for the back-and-forth. :(

Thanks for your work xjm. If it gets us further away from release, we should probably hold it off.

Although the UX is greatly improved when it comes to content creation/editing. It won't do a whole lot for configuration. The confidence that people have in Drupal is low, because of the many checkboxes that kill. Enabling revisions will give some alleviation on the content front (which is a big win), but not really solve the issue pictured by webchick in #21.

If its enabled by default, we might want to revist its interface a little too.

TL;DR +1

I can't recall a site I've built in the last 4years that doesn't have revisions enabled by default. Often the check box is forced on and hidden to remove yet another UI options issues similar those flagged by webchick.

For larger sites users want this functionality so they have the history of their content changes. Often this is needed for compliance and audits. I turn this on for smaller sites too because it is often these users who stuff something up and need a way of rolling back.

I think using negative performance implications to reject tim's proposal will come back to bite Drupal. Any performance issues with revisions on large sites are likely to become evident regardless as it is usually larger sites that use revisions.

I say we do this and deal with the bottlenecks as they're identified.

I think using negative performance implications to reject tim's proposal will come back to bite Drupal. Any performance issues with revisions on large sites are likely to become evident regardless as it is usually larger sites that use revisions.

Performance issues are already evident in D7, so a big -1 for auto enabling revisions without refactoring the revision system and its storage. Some of the arguments saying that enabling this will just make the other revision issues more prominent seem wishful and kicking the can down the road. Perhaps, revisions should be fixed together with this issue. :)

some history:

• #2083451: Reconsider the separate field revision data tables
• #1279440: Deeply decouple revisions from Entity API and Node module
• https://www.drupal.org/project/field_sql_norevisions

Just a note to say that this is on my to do list, but as a normal task it is down towards the bottom. I should be able to get to it next week, and will try for before.

I will say my gut though is to bump this to 8.1.x. This falls into the class of patches that seems simple, but could have great consequences (e.g. #35) and we do not need to open a new front on "stuff we gotta clean up" before shipping D8 at this point.

@giorgio79

Performance issues are already evident in D7, so a big -1 for auto enabling revisions without refactoring the revision system and its storage

IMO we have no issues with performance of revisions in D8 (it's obviously going to be a tick slower than not having revisions, but we have no issues with revision performance in D8). But even if there were, simply changing the default value of a checkbox will not make it any worse and shouldn't impact our decisions to configure saner defaults.

@webchick

I will say my gut though is to bump this to 8.1.x. This falls into the class of patches that seems simple, but could have great consequences (e.g. #35)

While that might be true, it also falls into the class of patches that are simple and deliver really great value :)

I really hope this change can make it into 8.0.x.

+1 on getting this into 8.0.x if possible. Since it's just a default value we're changing it shouldn't be a big deal.

@dixon have you read the links I posted in #35? Has the revision system been changed since D7? If not then this analysis still stands: http://posulliv.github.io/2013/01/08/norevisions-field/ and this performance is essentially due to the sql table structure, as well as the faulty logic of storing the exact duplicate values regardless of enabled or disabled revisioning as explained by the CCK creator #2083451: Reconsider the separate field revision data tables.

IMHO revisions are a critical feature of Drupal and I can't imagine building a website that didn't use them, so if there is a problem with revisions then Drupal is broken. I'm not saying there aren't performance issues to be addressed, but my issues in using revisions have been less about performance and more about developers forgetting to account for revisions in their modules. Out of sight, out of mind. Revisioning content by default excites me because—in addition to the possible usability improvement—it will force more developers to consider how their code works with revisions.

As has been pointed out, this issue isn't really going to change anything when it comes to performance. This isn't creating any new tables, or changing any queries. It just means that new users are more likely to use revisions and, since they can probably benefit from them more than most, that seems like a good thing. People who want to disable revisions still can, at least to the extent currently available without this change.

+1 to the change and +1 to doing it in 8.0.x.

I get the same failures locally on HEAD without the patch applied, let's try a re-test.

My preference is still to push this to 8.1.x. It's not going to get D8 to a shippable state sooner, and I have not seen any rebuttals to the concerns @giorgio79 is raising.

@webchick, here's a restatement of my rebuttal from above, trying to be more explicit.

While I do not wish to minimize the concerns @giorgio79 is raising, they are largely irrelevant to this discussion. There are certainly performance issues associated with revision support, whether they're enabled, or not, but this decision isn't going to change that one way or the other. Even if we don't enable revisions by default and you leave them turned off, Drupal is still going to create revision tables and save duplicate copies of your data. This won't change that. We can (and have, and will continue to) discuss changing Drupal's underlying architecture to enable the complete removal of revisions when not needed, in order to realize those performance gains, but that is a completely different issue.

Ultimately this is about user experience. The only thing we're talking about is changing the decision from "should I enable revisions on this node," to "should I disable revisions on this node?" The question before us is which approach will provide the best user experience out of the box? Experience tells me that enabling revisions by default would provide a net UX improvement and I haven't seen anything here to refute that hypothesis. Experience also tells me that it would be easier to do this for 8.0 then to wait for 8.1. So if committing this change wouldn't actually delay shipping 8.0 and it doesn't actually introduce any new performance issues, then it seems like it should be committed.

+1 on what jstoller said.

I read that, yes. It doesn't respond specifically to the concerns that @giorgio79 is raising (instead it calls them "irrelevant"), especially in relation to this issue's impact on actually shipping Drupal 8. He is saying there are fundamental structural problems with the way in which we store revisions. At the moment, that's a compartmentalized problem because revisions are an opt-in feature, so only sites that do something will run into these issues, and therefore they are things we can take our time about thinking about how to fix, and do it in the right way.

If we were to put this feature in now, then suddenly all sites are affected by the problems he's describing, and now we are severely at risk for adversely affecting D8's timeline, especially if fixing these problems requires the kind of fundamental architecting that seems to be alluded to about here.

So unless some research can be done to conclude that those concerns are off-base, and the problem's totally manageable, I see no reason to destabilize 8.0.x for this, when it's literally a checkbox away for site builders to enable this for themselves if they want it.

Alternately, some credible arguments for why if we don't do this in 8.0, and instead pushed it to 8.1, which is safer, we would be causing consequences dire enough to potentially further delay D8.0. I can't think of any myself, despite the fact that I'd love to commit this from a usability POV. The beta evaluation itself says "Normal - There is no rush to get this out, but it would be great to get it in 8.0.x to start off in a good place. " so the issue summary itself is making the argument that this is a feature, and features were postponed to 8.1.x long ago.

It doesn't respond specifically to the concerns that @giorgio79 is raising (instead it calls them "irrelevant")...

Again, I'm not in any way trying to minimize @giorgio79's concerns. In the grand scheme of things they are very relevant, but I just don't see how they're relevant to the issue at hand. Please don't read any disrespect into my statement as none was intended.

...especially in relation to this issue's impact on actually shipping Drupal 8.

If there is a justifiable fear that this patch could reasonably place another delay on shipping D8, then I'm all for waiting. I just haven't seen anything indicating that is the case.

He is saying there are fundamental structural problems with the way in which we store revisions.

Yes, but he is not comparing the performance of having revisions enabled vs. disabled. He is comparing the performance of simply having revision tables with duplicate data vs. an alternate field storage system that doesn't use the revision tables at all. So again, this doesn't seem terribly relevant to this discussion. Whether we enable them by default or not, Drupal is still going to create revision tables with duplicate data, so we're getting that performance penalty anyway. Not committing this change isn't going to magically make things better for people who aren't going to use revisions anyway.

At the moment, that's a compartmentalized problem because revisions are an opt-in feature, so only sites that do something will run into these issues, and therefore they are things we can take our time about thinking about how to fix, and do it in the right way.

I'm not sure I can get behind this sentiment. Revisions are a major feature of Drupal and I personally have never built a site that didn't need them. If revisions are broken, then Drupal is broken. I certainly won't be able to use D8 until they work. Further, I would argue that most of the people who are using Drupal without revisions are doing it wrong, but perhaps that's a discussion for another time.

So unless some research can be done to conclude that those concerns are off-base, and the problem's totally manageable, I see no reason to destabilize 8.0.x for this...

Again, I can't see how the particular concern raised by @giorgio79 applies here. Other possible areas of concern were evaluated earlier in this issue and it was determined that they're not very concerning. So far no one has presented anything indicating that this would in any way destabilize 8.0.x. Conversely, a number of people have indicated that it would be a UX win.

...when it's literally a checkbox away for site builders to enable this for themselves if they want it.

And if we made the change it would literally be a checkbox away for site builders to disable revisions, if they want. The question is, are the people who don't understand that choice more likely to benefit from having the revisions enabled or disabled by default? I think the former.

@webchick, please don't feel you need to reply to all this. I just hope you take it under consideration when evaluating this issue.

Since we are all in agreement about the UX win here, isolating response to the 8.0.x destabilization question, since that's the big "things that make you go hmmm" for me.

Other possible areas of concern were evaluated earlier in this issue and it was determined that they're not very concerning. So far no one has presented anything indicating that this would in any way destabilize 8.0.x.

This concern was raised by at least xjm in #29 and giorgio in #35. I have not seen anyone say "No worries, I spent time going through each of the major issues that affect node revisions in detail and they present no release risk. I've also profiled with 1000 nodes with 1000 revisions each, and no crushing performance issues ensued which would create more critical issues." If I missed that analysis, then great. If not, though, I feel like the people pushing for this feature to go in before 8.0.0 need to do some work to demonstrate the lack of risk. We're less than 10 issues away from an RC now.

I have been following this issue with interest. As a Drupal developer, I am very eager to see the first Drupal 8 Release Candidate. As a management consultant, I see the operational requirements of companies that will implement Drupal 8. Please allow me to share this very different perspective:

Most successful companies today are certified with a Quality Management System. ISO 9001 certification for example, requires document management. Given the importance of a website for a company, document management and full traceability are not only basic requirements for general operations, but also for the CMS.

After having been responsible for several ISO 9001 certifications, I can share with you that traceability, or revision management, has a higher priority than performance for most companies. In our organizations revision management was implemented first, then the performance of the respective processes was improved.

I would hope that the same approach would apply to Drupal 8. Revisions are a must. Revisions should be on by default. If architectural changes need to be made to accommodate revisions in a more effective manner, than these should be implemented now - before many clients get on the Drupal 8 train and not at a later stage. If this delays the release of RC1 by a few days (is there a set release date?), than I am all for it as it results in a better, more competitive product. In practice, revisions will be one more argument for selling Drupal 8 to prospective clients.

Andreas

@platinum You should note that this question is not about inclusion of exclusion of revisions, but weather they should be enabled by default or not. All who makes Drupal 8 sites, can do so with revisions on nodes, but they will need to click a checkbox in order to do so. There are two general opinions in this regard.

1. Have it enabled by default, as those who are unsure about this, would gain a lot of benefit having it turned on, to avoid loss of content.
2. Having it turned on by default is a bad idea, since the system isn't working that well (performance) and having it turned on will require us to have a better revision system.

Having to build a "good" revision system probably will take a long time, maybe months so that's not a viable option. A lot of us, still feel that revisions should be turned on by default, since it's a huge win, if you don't know about this and if you know about this, then you can easily turn it off.

@googletorp I understood the question. That is why I wrote "Revisions are a must. Revisions should be on by default". To reiterate, I vote revision enabled by default +1.

I was trying to share a non-developer perspective on the topic, with some real world information to support my view. From this perspective, I would even go one step further: Enable revisions by default and only allow the administrator to disable revisions via a configuration setting. The user generating content should not be able to choose if revisions are enabled or not. Removing the "Create new revision" checkbox would also simplify and improve the UX :-)

Edit: for clarity

At the Entity API BoF in Barcelona we agreed that revisions should be the default for nodes. Patch is green. Let's do this.

@webchick was pretty clear in #57 and we should have solid arguments for it. The "people enable it anyway" is not really an argument? Since if we benchmark reality, Drupal is probably quite worse of :)

This is getting really frustrating. I tried to say, at least 3 times, that what needed to get done before this got committed was someone to look into the outstanding issues around revisions, and verify that they would not create release blockers were this option enabled by default. Instead, these requests get ignored, and the exact same arguments that I've already said I agree with around UX get repeated over and over.

I am unassigning from myself and unsubscribing. I won't block commit here, but have to say I am pretty frustrated with the tactics that are being used here to drive this feature change in during the bug-fixing phase and run-up to release. I probably should've just moved it to 8.1.x from the outset, but was trying to give people a window of opportunity to do the right thing.

+++ b/core/modules/book/config/install/node.type.book.yml
@@ -8,6 +8,6 @@ name: 'Book page'
+new_revision: true

We are changing the new_revision to TRUE in the default value. So do we need a CR for that ? b/c we are updating the existing functionality.

Steps:

1. Revision is enable by default. - checked it on article and basic page.
2. I have created custom content type. And tested the revision on that content type also. worked fine for me
3. Created a custom block and checked revision there also.

One obvious thing is to provide some up to date profiling numbers, how much slower is saving an entity with and without revision support? How much slower is loading an entity which has revision support etc. Especially some scaling numbers would be great I think.

In general all problems which might could exist should be resolved in general.

Here are some of the important comments:

* https://www.drupal.org/node/2490136#comment-10084878
* https://www.drupal.org/node/2490136#comment-10294751
* https://www.drupal.org/node/2490136#comment-10095786

I reformatted Tim's results for easier reading. The numbers here are a bit inconclusive IMO. I think we should proceed, as the this issue is one building block of many towards http://buytaert.net/improving-drupal-content-workflow. That's a major improvement to our core CMS functionality.

So the other part which was pointed out by @webchick earlier is to do a review of the existing issues around revisions. Are there some which make it problematic to ship with it by default?

Core currently does not use forward revisions at all, which means various things are buggy, as we've found working on Workbench Moderation. For example: #2708735: Creating a draft removes published node from taxonomy view.

Non-forward revisions have been fairly stable for me since Drupal 7.

The API is also incomplete on non-nodes, but that doesn't impact enabling node revisions by default (which I support).

The API is also incomplete on non-nodes, but that doesn't impact enabling node revisions by default (which I support).

Right, because the default node UI builds around the idea of using never forward revisions.

Re #92: I mean more that there are revision-handling methods on Node and NodeType that are not on any other entity type, including block content, which does support revisions in core. But that shouldn't restrict us from enabling revisions on nodes by default.

@timmilwood With #91 Could you do a quick sum of that (with issue links) to evaluate?

Patch in #79 works as expected for me.

I think the way we might unearth any UI oddities is by committing this to 8.2.x now. we can't proceed in the content workflow initiative without this and existing sites will continue to not have revisions unless they change their configuration.

Given that this issue is mostly about the discussion giving everyone credit who commented.

Committed 0f3964d and pushed to 8.2.x. Thanks!

This change has impact on more than the UI itself. For example:

• #1239558: Deleting an entity with revisions and file/image field does not release file usage of non-default revisions, causing files to linger
• #2465901: [META] Make entity revision translation work (maybe mostly addressed?)
• #2362435: When viewing a revision, the Quick Edit, Edit, and Delete contextual link operations are available, but should not be / #2389341: When viewing a revision the Reverted or Deleted links should be available in contextual links
• #2708735: Creating a draft removes published node from taxonomy view

Given that we have a checkbox to make a new revision on the node form for site admins, I don't think it defaulting to on or off for new content types significantly increases the impact of those bugs vs. not - we need to fix them either way. There are also years of data-loss bugs, especially in conjunction with 7.x workflow modules and/or entity translation from the behaviour of sometimes saving a revision and sometimes not, and while this patch doesn't stop that either, we need to get to the point where we never save a revisionable entity without it being a new revision - i.e. rather than just switching the default, remove the option from the UI altogether and heavily deprecate it in the API.

From the list, #2708735: Creating a draft removes published node from taxonomy view is not affected by this, since there's no ability to make forward revisions in core yet, it's just a bug with core revision support that's exposed by contrib modules (which have to create new revisions in order to create new drafts at all).

This change seems worth calling out in the release notes.

Published change record