Problem/Motivation

Revisions are not enabled by default in Drupal for both new content type and the article / page content types added by the standard install profile. Revisions are a good practice so should be default. I would go as far as saying they should not be optional, but we'll leave that for Drupal 8.1.x

Proposed resolution

Enable revisions by default when creating a new content type, and also for the article and page content types added by the standard install profile.

Remaining tasks

Needs a beta evaluation update to include what revisioning bugs, if any, would now be a part of the out of the box experience.

Signoffs

  • Product manager: @webchick seemed to be in favor of this change in #21.
  • Entity Field API: @berdir said this change would be okay (see #22)

User interface changes

"Create new revision" checkbox is checked by default.
Change record - https://www.drupal.org/node/2491507

API changes

None

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Feature because it updates the "Create new revision" checkbox to follow better practices for site builders.
Issue priority 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.
Prioritized changes The main goal of this issue is usability

Comments

mikeburrelljr’s picture

Status: Active » Needs work

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
mikeburrelljr’s picture

@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’s picture

Status: Needs work » Needs review
FileSize
1.94 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,938 pass(es), 1,058 fail(s), and 125 exception(s). View

This now updates the default for new content types to be revisionable as well as the standard article and page content type.

Status: Needs review » Needs work

The last submitted patch, 3: enable_revisions_by_default-2490136-1.patch, failed testing.

Status: Needs work » Needs review
cilefen’s picture

Issue tags: +Needs change record

Status: Needs review » Needs work

The last submitted patch, 3: enable_revisions_by_default-2490136-1.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.51 KB
FAILED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,501 pass(es), 2 fail(s), and 0 exception(s). View

Status: Needs review » Needs work

The last submitted patch, 8: enable_revisions_by_default-2490136-2.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
2.23 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 97,067 pass(es). View
cilefen’s picture

@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’s picture

Issue summary: View changes
timmillwood’s picture

timmillwood’s picture

Issue summary: View changes
Issue tags: -Needs change record
cilefen’s picture

@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.

timmillwood’s picture

Issue summary: View changes

@cilefen good thinking, updated the change record.
Just waiting on RTBC now.

naveenvalecha’s picture

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.

timmillwood’s picture

FileSize
3.33 KB
FAILED: [[SimpleTest]]: [PHP 5.5 MySQL] Unable to apply patch enable_revisions_by-2490136-19.patch. Unable to apply patch. See the log in the details link for more information. View

Added the suggestions from #18.

dixon_’s picture

Status: Needs review » Reviewed & tested by the community

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.

webchick’s picture

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.

xjm’s picture

Category: Feature request » Task
Issue summary: View changes
Status: Reviewed & tested by the community » Needs review
Issue tags: +Usability, +Needs beta evaluation, +Entity Field API

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.

xjm’s picture

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. :)

xjm’s picture

Issue summary: View changes
timmillwood’s picture

xjm++ Thanks for the efforts / discussions here.

Taking a look in the queues there are 10 major bugs for D8 that mention "revisions", I'll look through these to see what the impact may be.

xjm’s picture

Issue tags: +D8 Accelerate London

(Forgot to tag yesterday.)

jhedstrom’s picture

Issue tags: -Needs beta evaluation

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.

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

@jhedstrom - The search I linked to only looked at "major". Looking at "critical" items of any category that mention revisions I don't see any concerns.

There are 43 issues currently that are classified as "major" and have a mention of "revisions". I don't feel any of these should be blockers to this patch, so with that and the +1 from #27 I am switching this back to RTBC.

xjm’s picture

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

Maybe we should do some profiling?

And then:

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.

xjm’s picture

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

xjm’s picture

Assigned: timmillwood » webchick

@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. :(

Bojhan’s picture

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.

timmillwood’s picture

Thanks for your support @Bojhan, I think enabling revisions would be good for 8.0.x then revisiting the interface in 8.1.x

It'd be great to see something like multiversion get into core around 8.1.x or 8.2.x

skwashd’s picture

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.

giorgio79’s picture

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:

webchick’s picture

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.

dixon_’s picture

@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.

googletorp’s picture

+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.

Anonymous’s picture

It sounds as though any performance issues are going to exist whether or not the option is defaulted to enabled and could bite a project at any point if not addressed making that a separate yet essential issue.

By definition, this is a pre-existing that does not introduce any new functionality and yet ushers in a lot of new and valuable work to be done in subsequent releases. Specifically I am a huge fan of the core conversation from LA (CRAP everywhere) about systems calling themselves a CMS taking "content" much more seriously and carefully. I understand the need to not take it lightly. There are schema implications but everything already exists ... nothing new. These schema implications also make me think a D7->D8 transition is a much more logical time to make this adjustment.

Can we say that the performance issues go away by not setting this default? I ask, as I wonder if there is an un-stated alternative that I'm not clear on reguarding how we will be releasing D8.

Perhaps this simple default, which introduces no new functionality, brings visibility and prominence to a performance issue that we can get in front of.

timmillwood’s picture

@xjm - you mentioned #2261669: Slow query in NodeRevisionAccessCheck as being one of the issues you were most concerned about, so I re-rolled the patch for it.

giorgio79’s picture

@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.

timmillwood’s picture

jstoller’s picture

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.

timmillwood’s picture

@webchick - I just wanted to quickly ping you again on this to see if it's made it's way up your todo list?

While waiting I've been looking at some of xjm's suggestions in #29 which should help the cause.

Seeing as revisions is a best practice (so most people will / should enable it anyway) and Wordpress enabled revisions by default, I think it should be a no-brainer and maybe even a higher priority.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 19: enable_revisions_by-2490136-19.patch, failed testing.

amateescu’s picture

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

Status: Needs work » Needs review

timmillwood’s picture

Status: Needs review » Reviewed & tested by the community

Looks like this is still passing tests.

timmillwood’s picture

@xjm & @webchick - Is there any chance of getting this into 8.0.x? It'd be awesome if we can is it brings functionality which all developers / site builders should enable to a default.

webchick’s picture

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.

jstoller’s picture

@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.

googletorp’s picture

+1 on what jstoller said.

webchick’s picture

Status: Reviewed & tested by the community » Needs review

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.

jstoller’s picture

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.

timmillwood’s picture

@jstoller - Thanks for the support of this patch, awesome posts.

I still stand by:

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.

I feel that "good place" is key for those starting to build on 8.0.x, if we roll this patch out on 8.1.x only new content types created will have revisions by default, old content types may not have revisions. I fear this may cause more confusion than the UX issues it's trying to overcome. @webchick I think it was you that pointed out earlier in this thread that enabling revisions was something that tested badly in UX testing. So yes it is "literally a checkbox away", but it seems they're too scared / nervous / unsure to check it.

webchick’s picture

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.

timmillwood’s picture

One of the two "most concerning" xjm outlined in #29 have already been closed and I will be spending some time to try to resolve some more.

I would love to see the multiversion module make it's way (partly or fully) into core to replace the current revisioning system.

My concern with the current arguments is, yes there are performance issues, but they are still there whether we enable revisions by default or not. Any savvy Drupal developer / site builder will be enabling revisions anyway, especially on the larger sites which are the ones effected by the performance issues. The type of users who won't enable revisons are the ones who don't know better or who are scared about what the feature really is, these type of users are often building smaller low load sites and may not ever see or notice these performance issues.

platinum1’s picture

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

googletorp’s picture

@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.

platinum1’s picture

@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

timmillwood’s picture

For those interested in this topic @dixon_ and I will be presenting Planning for CRAP and entity revisions everywhere in core at Drupalon Barcelona. Enabling revisions by default is one of the first steps and there are so many more which @dixon_ has spearheaded via contrib, it'd be awesome to get some of this trickling into core from 8.1.x onwards.

Status: Needs review » Needs work

The last submitted patch, 19: enable_revisions_by-2490136-19.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
3.28 KB
PASSED: [[SimpleTest]]: [PHP 5.5 MySQL] 113,986 pass(es). View

Re-rolling

dixon_’s picture

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

dixon_’s picture

Status: Needs review » Reviewed & tested by the community
Bojhan’s picture

Status: Reviewed & tested by the community » Needs work

@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 :)

timmillwood’s picture

Status: Needs work » Reviewed & tested by the community

- It provides a better user experience
- It displays best practices
- It allows content authors to truly manage their content
- It sets a good direction for D8

And... I really think "people enable it anyway" is a great argument. It makes life easier for site builder, and a better experience for content editors.

I would love to hear if @webchick has an updated view on this so resetting @dixon_'s RTBC.

webchick’s picture

Assigned: webchick » Unassigned

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.

Bojhan’s picture

Status: Reviewed & tested by the community » Needs work
timmillwood’s picture

@webchick - Sorry, I heard you'd been thinking about / discussing this issue and thought it might be ready.

We've been looking at issues outlined by @xjm in #29 so think we're close on the outstanding issues, and I truly believe there is nothing here that would block the RC1 release.

The initial phase of the planned revisions work is this issues, as outlined in my Drupalcon session (https://youtu.be/LKQczUM7Qrw?t=13m53s). The main reason for getting this into core for 8.0.0 is that in 8.1.0 we can introduce revisions by default across all content entities without it seeming an alien concept.

timmillwood’s picture

Version: 8.0.x-dev » 8.1.x-dev
Status: Needs work » Needs review
timmillwood’s picture

Queued for retesting, revisiting for 8.1.x

Status: Needs review » Needs work

The last submitted patch, 65: enable_revisions_by-2490136-65.patch, failed testing.

The last submitted patch, 65: enable_revisions_by-2490136-65.patch, failed testing.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.0-beta1 was released on March 2, 2016, which means new developments and disruptive changes should now be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

The last submitted patch, 65: enable_revisions_by-2490136-65.patch, failed testing.

timmillwood’s picture

Updating invalid tests

timmillwood’s picture

If anyone has chance to review, that'd be awesome!

naveenvalecha’s picture

+++ 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.

timmillwood’s picture

The change record is at https://www.drupal.org/node/2491507

Manjit.Singh’s picture

FileSize
94.66 KB
84.73 KB

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.

content

content

Manjit.Singh’s picture

Status: Needs review » Reviewed & tested by the community
timmillwood’s picture

@xjm @alexpott @webchick - I think the issue for me is understanding the blockers for this. Do you mind giving a quick overview? I will then be able to work on resolving them.

dawehner’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs profiling

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

timmillwood’s picture

Issue tags: -Needs profiling

I wrote a very basic performance test which creates 50 Node entities and 50 BlockContent entities, it then updates all of them 50 times. I do this once with setNewRevision(true) and once with setNewRevision(false). I then load each 50 entities by title.

I ran the script 3 times, here's the results:
Node with revisons, 23.054795980453ms to create and update, 0.078958988189697ms to load
Node without revisions, 23.71729183197ms to create and update, 0.075845003128052ms to load
BlockContent with revisions, 18.293105840683ms to create and update, 0.070639848709106ms to load
BlockContent without revisions, 18.271573066711ms to create and update, 0.093148946762085ms to load

Node with revisons, 22.560276031494ms to create and update, 0.063925981521606ms to load
Node without revisions, 23.404742002487ms to create and update, 0.066656112670898ms to load
BlockContent with revisions, 19.950647830963ms to create and update, 0.071696996688843ms to load
BlockContent without revisions, 19.126659870148ms to create and update, 0.11081290245056ms to load

Node with revisons, 23.061083078384ms to create and update, 0.065523862838745ms to load
Node without revisions, 23.849312067032ms to create and update, 0.070587158203125ms to load
BlockContent with revisions, 19.585115909576ms to create and update, 0.07262110710144ms to load
BlockContent without revisions, 19.11677479744ms to create and update, 0.11151790618896ms to load

As you can see the difference is so small, and this could just be noise.

I've posted by script on https://gist.github.com/timmillwood/ed75cbf4b066db8777bd7aa333d2db5a

moshe weitzman’s picture

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.

Create+Update Load
Revision No-revision Revision No-revision
Node #1 23.05 23.72 .079 .076
Node #2 22.56 23.44 .064 .067
Node #3 23.06 23.85 .066 .071
BlockContent #1 18.29 18.27 .071 .093
BlockContent #2 19.95 19.13 .072 .111
BlockContent #3 19.59 19.12 .073 .112
dawehner’s picture

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?

Crell’s picture

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).

timmillwood’s picture

Re: #89 - After a quick look I can't see any major issues relating to revisions which should restrict them from being enabled by default.

All of the issues I see look to be more relating to UX, and I personally believe that enabling revisions by default gives a better user experience and out weighs any negative implications.

dawehner’s picture

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.

Crell’s picture

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.

Bojhan’s picture

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

timmillwood’s picture

Issue tags: +Workflow Initiative
markdorison’s picture

Patch in #79 works as expected for me.

markdorison’s picture

Status: Needs review » Reviewed & tested by the community
alexpott’s picture

Status: Reviewed & tested by the community » Fixed

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!

  • alexpott committed 0f3964d on 8.2.x
    Issue #2490136 by timmillwood, Manjit.Singh, xjm, webchick, dixon_,...
catch’s picture

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).

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

webchick’s picture

Issue tags: +8.2.0 release notes

This change seems worth calling out in the release notes.