Problem/Motivation

See #2725447: WI: Phase D: Enable archive/purge storage

Proposed resolution

  • Add a status field type with existing unpublished/published options plus archived option.
  • Update node and comment entity types to use the new field type
  • Add the field to all revisionable entity types
  • Create an upgrade path for all views to use the new field type.

Remaining tasks

Code.

User interface changes

None.

API changes

No API changes. Additions are described in the proposed resolution.

Data model changes

One additional base field for all revisionable content entities. See the proposed resolution.

Files: 

Comments

dixon_ created an issue. See original summary.

dixon_’s picture

Issue summary: View changes
dixon_’s picture

Status: Active » Postponed

Marking this as postponed since sign-offs are pending in #2725447: WI: Phase D: Enable archive/purge storage.

timmillwood’s picture

Status: Postponed » Needs review
FileSize
2.73 KB

Getting the ball rolling by adding the archived field to revisionable entities..

Status: Needs review » Needs work

The last submitted patch, 4: wi_add_archive_and-2725463-4.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.39 KB
3.22 KB

Status: Needs review » Needs work

The last submitted patch, 6: wi_add_archive_and-2725463-6.patch, failed testing.

timmillwood’s picture

Fixing tests

timmillwood’s picture

Status: Needs work » Needs review
Wim Leers’s picture

+++ b/core/modules/block_content/block_content.install
@@ -61,3 +61,18 @@ function block_content_update_8003() {
+    ->setDescription(t('Indicates if the entity is archived or not.'))

+++ b/core/modules/node/node.install
@@ -218,3 +218,18 @@ function node_update_8003() {
+    ->setDescription(t('Indicates if the entity is archived or not.'))

Shouldn't this use the specific entity type name?

timmillwood’s picture

Lots of new stuff.
Added setArchived and isArchived methods, added archived to buildQuery so archived entities don't get loaded, and added tests.

Status: Needs review » Needs work

The last submitted patch, 11: wi_add_archive_and-2725463-11.patch, failed testing.

amateescu’s picture

  1. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -332,6 +332,19 @@ public function getRevisionId() {
    +    $field_name = 'archived';
    +    return ($this->hasField($field_name)) ? $this->get($field_name)->value : FALSE;
    ...
    +    $field_name = 'archived';
    +    if ($this->hasField($field_name)) {
    +      $this->set($field_name, $archived);
    +    }
    +    return $this;
    

    Let's skip initializing a $field_name var.. it's kinda useless.

  2. +++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
    @@ -1132,6 +1145,12 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
    +        ->setTranslatable(TRUE)
    

    Why do we want this field to be translatable? Without this we cannot archive a specific translation of an entity?

  3. +++ b/core/lib/Drupal/Core/Entity/Sql/SqlContentEntityStorage.php
    @@ -671,6 +672,10 @@ protected function buildQuery($ids, $revision_id = FALSE) {
    +        $query->condition('revision.archived', FALSE);
    

    Boolean fields store 1 and 0 in the database, so let's use 0 here.

  4. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentArchiveTest.php
    @@ -0,0 +1,51 @@
    +class BlockContentArchiveTest extends KernelTestBase{
    
    +++ b/core/modules/node/tests/src/Kernel/NodeArchiveTest.php
    @@ -0,0 +1,51 @@
    +class NodeArchiveTest extends KernelTestBase{
    

    I'm pretty sure we can add these test methods to some existing test classes...

  5. +++ b/core/modules/block_content/tests/src/Kernel/BlockContentArchiveTest.php
    @@ -0,0 +1,51 @@
    \ No newline at end of file
    
    +++ b/core/modules/node/node.install
    @@ -218,3 +218,16 @@ function node_update_8003() {
    \ No newline at end of file
    
    +++ b/core/modules/node/tests/src/Kernel/NodeArchiveTest.php
    @@ -0,0 +1,51 @@
    \ No newline at end of file
    

    Some missing empty lines :)

  6. +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityDefinitionUpdateTest.php
    @@ -102,6 +102,7 @@ public function testEntityTypeUpdateWithoutData() {
    +        t('The %field_name field needs to be installed.', ['%field_name' => $this->entityManager->getFieldDefinitions('entity_test_update', null)['archived']->getLabel()]),
    

    null -> NULL

amateescu’s picture

+++ b/core/lib/Drupal/Core/Entity/ContentEntityBase.php
@@ -1132,6 +1145,12 @@ public static function baseFieldDefinitions(EntityTypeInterface $entity_type) {
+      $fields['archived'] = BaseFieldDefinition::create('boolean')
+        ->setLabel(new TranslatableMarkup('Archive flag'))
+        ->setRevisionable(TRUE)
+        ->setTranslatable(TRUE)
+        ->setDefaultValue(FALSE);

And the most important part.. how about we only add this base field if the entity uses a special sql storage class/interface, (e.g. SqlEntityStorageNGInterface or SqlEntityStorageArchiveInterface)

timmillwood’s picture

Status: Needs work » Needs review
FileSize
10.29 KB
7.22 KB

Fixing items from #13 and fixing tests.

Not sure I agree with #14, will discuss with @amateescu.

Status: Needs review » Needs work

The last submitted patch, 15: wi_add_archive_and-2725463-15.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
1.8 KB
10.21 KB

Fixing broken tests.

Discussed with @amateescu about #14. This all depends how we re-implement the delete() method, or add archive(), or add purge(), etc. I think this should be broken out into a sub-issue to discuss.

The attached patch can go in now without breaking anything, but adding a lot of awesome functionality. As seen in the test it can be used by doing $entity->setArchived(TRUE)->save(). The follow up issue will then decide if delete() sets something to archived, or not, and what else we add to bring this functionality to the UI.

timmillwood’s picture

Title: WI: Add archive and purge to the entity storage API » WI: Add archived base field to revisionable entities
Issue summary: View changes
benjy’s picture

Do we really want this on ContentEntityBase? Seems like something the Trash module or whatever is using it should be adding?

timmillwood’s picture

@benjy - I'd like to see it as a general core feature. Every revisionable entity gets archived (not deleted), and then purged if it needs to be removed for ever. Trash module is simply a UI for restoring archived entities, and purging archived entities.

Adding it in ContentEntityBase alongside the revision field makes sure it's added to all revisionable entities.

timmillwood’s picture

Status: Needs review » Needs work

After speaking to @catch there's concern about adding an extra condition to every entity query. I can understand the concern, but can't think of a better solution. There was talk about maybe converting the status field to -1:0:1 to denote archived,unpublished,published. This kind of solution was also talked about for denoting if a revision is a draft / forward revision, this would allow for having a default (live) revision newer than a forward (draft) revision, so do we then add -1:0:1:2? The main issue here is not all entities have a status field, so we would have to add that in for all, but that's no different than adding an archived field.

amateescu’s picture

The main issue here is not all entities have a status field, so we would have to add that in for all, but that's no different than adding an archived field.

It is in fact very different, because the node's 'status' field is already taken into account everywhere in the code base, every query is already optimized to death, etc. etc. :) So yeah, adding a status field to every other entity type is the way to go, IMO.

It would probably need to be a new field type with some methods that are specific to its tri-state architecture in order to provide a better DX than a simple integer field.

timmillwood’s picture

It would probably need to be a new field type with some methods that are specific to its tri-state architecture in order to provide a better DX than a simple integer field.

This sounds like an awesome solution. I'd propose it as a quad-state field though:
-1: Archived / Deleted
0: Unpublished
1: Published
2: Draft / Forward revision / Never been published

0 and 1 will work exactly as the current status boolean field works.
-1 will give the features this issue is looking for.
2 will help address an outstanding core issue with forward revisions discovered in #2766957: Forward revisions + translation UI can result in forked draft revisions. With this status we will be able to mark a revision as what we've historically called a "forward revision", but allow these to exist anywhere within the life of an entity.

I have a slight concern about backwards compatibility and API breaking changes, however as long as $node->status behaves like it always has, then adding a -1 and 2 value should be ok.

timmillwood’s picture

Title: WI: Add archived base field to revisionable entities » WI: Add StatusItem field type to store archived state
Status: Needs work » Needs review
FileSize
24.77 KB

Making a start.

Status: Needs review » Needs work

The last submitted patch, 24: wi_add_archived_base-2725463-24.patch, failed testing.

The last submitted patch, 24: wi_add_archived_base-2725463-24.patch, failed testing.

The last submitted patch, 24: wi_add_archived_base-2725463-24.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
2.86 KB

Here is a start of some views integration

timmillwood’s picture

Thank you so much @dawehner for making a start on the Views stuff, and cleaning up my messy patch.

Turns out I diffed against 8.2.x instead of 8.3.x.

Anyway, this new patch updates the status field definitions on Node and Comment.

Status: Needs review » Needs work

The last submitted patch, 29: wi_add_statusitem-2725463-29.patch, failed testing.

dawehner’s picture

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

.

The last submitted patch, 29: wi_add_statusitem-2725463-29.patch, failed testing.

The last submitted patch, 29: wi_add_statusitem-2725463-29.patch, failed testing.

timmillwood’s picture

Issue summary: View changes

After speaking to @dawehner about this issue it seems the views upgrade path is complex. In theory it sounds like we can update all views ever to use the new field, but this doesn't seem to sit right with me, also I have no idea how to do it.

Then spoke to @catch and he expressed concern about using 2 to denote a draft/forward revision, which is not strictly in scope of this issue, but seemed a good win. The issue is when people do if ($node->status) expecting a published node, this would actually return published and draft nodes. One option here would be to use -2 for draft/forward revisions.

dawehner’s picture

Even that doesn't help. Negative numbers are evaluated to true, see https://3v4l.org/1UkAo

The last submitted patch, 29: wi_add_statusitem-2725463-29.patch, failed testing.

timmillwood’s picture

@dawehner interesting, so even -1 for archived won't work too well.

timmillwood’s picture

Haven't actively worked on this issue for 5 days now, but keep thinking about the point from #35.

Here's the dilemma:
- This issue was originally for creating a new archived field and querying that on every entity query. Although adding this new condition could be quite a performance hit.
- The second solution was upgrading the status field to have more than two possible values. However this means an extensive upgrade path and places in core and contrib that assume if ($entity->status) { 'entity is published.' } will break if we add further positive or negative numbers.

So I am stuck with working out what is the lesser of two evils.

Personally I am still quite keen on adding the new archived field, but then do we add a new draft field, a new foo field, a new bar field, where do we stop. Maybe we keep the current status field, and add a state field?

As a side note in the Multiversion module we add this functionality with a _deleted field, and the limited performance testing we've done shows the impact is minimal.

I'll try to discuss with @amateescu and @_dixon tomorrow.

amateescu’s picture

One idea that came to mind is to take advantage of the magic getters and return 0 when the actual status is -1. That would fix the if ($entity->status) { 'entity is published.' } case but it will probably break REST export. Maybe we could just manipulate the value to return -1 when we're in a "REST" context or something..

oriol_e9g’s picture

With the new release cycle we CAN break compatibility in Drupal 8.3, so simply add the new status codes, create a new change record and rewrite all occurrences of if ($entity->status) {... by if ($entity->status == 1) {...

I think the cleaner way is simply break old code compatibility.

I'm for:
-2: archived
-1: draft
0: unpublished
1: published

catch’s picture

I had a similar idea to #39 - keep ->status return as bool. Add a new ->isArchived() method which also returns bool. But populate both of these via one column with 0/1/-1.

REST goes via the serializer, so it ought to be possible to put the raw/non-bool value in the export, although not sure how straightforward/tricky that would be in practice.

@oriol_e9g if a module doesn't update, and then ends up showing unpublished content for some reason, that's too much of an API break for a minor IMO.

fwiw that's also one reason I'm concerned about the extra column. You'd need to add it to every entity query, except for ones that really want to return all entities, which would then need a killswitch similar to accessCheck(FALSE).

The problem then any entity update that uses entityquery and is expecting to run on all entities, would have to know to add the killswitch otherwise some will be missed. I regularly see (and have also forgotten myself) entity queries in drush commands or similar that forgot accessFalse, which then end up breaking when node access modules are enabled. The extra column would be another place that could go wrong, and since the entities are archived it could take a long time to figure out they've been missed.

amateescu’s picture

Spoke about this issue today with @dixon_ and @timmillwood, and Dick raised a valid concern about using the status field for storing the archived state: when you want to 'restore' the content, you won't know the original status (published/unpublished) before it was archived.

rachel_norfolk’s picture

Ideally, archived entities should be on display at the bottom of a locked filing cabinet stuck in a disused lavatory with a sign on the door saying beware of the leopard.

But failing that, I think catch and amateescu have it right in #39 and #41.

timmillwood’s picture

Adding / overriding methods on the Field Type.

Guess we need isArchived() on the entity too, just working out the logic for that.

Status: Needs review » Needs work

The last submitted patch, 44: wi_add_statusitem-2725463-44.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review

I'm starting to see an issue with keeping $entity->status as bool. For example in \Drupal\Core\Entity\Sql\SqlContentEntityStorage::mapToStorageRecord the line $value = isset($entity->$field_name->$column_name) ? $entity->$field_name->$column_name : NULL; which is basically calling $entity->status->value. If we force status to return a boolean value then the correct (-1) value will never get saved.

timmillwood’s picture

Progress patch.

Added NodeArchiveTest which tests isArchived() and setArchived().

Status: Needs review » Needs work

The last submitted patch, 47: wi_add_statusitem-2725463-47.patch, failed testing.

timmillwood’s picture

@chx @dixon_ @amateescu @alexpott and I discussed this on IRC and @chx came up with the suggestion of using sql views.

This might actually work. It would allow us to add new base fields for archived, and for other things like deleted, draft and workspace.

When doing an entity query it would query a view with non-archived, non-deleted entities. If wanting archived / deleted etc entities then it'll be possible to query a different view.

timmillwood’s picture

timmillwood’s picture

Never really used MYSQL VIEWs, so been having a quick play, these are cool, why haven't we used them before?

mysql> create view published as select * from node n join node_field_data nfd using (nid, vid, type, langcode) where nfd.status = 1;
Query OK, 0 rows affected (0.00 sec)

mysql> select * from published;
+-----+------+---------+----------+--------------------------------------+-------+-----+--------+------------+------------+---------+--------+-------------------------------+------------------+-----------+----------+------------------------------------+
| nid | vid  | type    | langcode | uuid                                 | title | uid | status | created    | changed    | promote | sticky | revision_translation_affected | default_langcode | workspace | _deleted | _rev                               |
+-----+------+---------+----------+--------------------------------------+-------+-----+--------+------------+------------+---------+--------+-------------------------------+------------------+-----------+----------+------------------------------------+
|   1 |    1 | article | en       | b43627b2-7014-4fe9-9634-081c32111800 | t     |   1 |      1 | 1471883658 | 1471883661 |       1 |      0 |                             1 |                1 |         1 |        0 | 1-f55d88a2e2786083620569465d259fc7 |
+-----+------+---------+----------+--------------------------------------+-------+-----+--------+------------+------------+---------+--------+-------------------------------+------------------+-----------+----------+------------------------------------+
1 row in set (0.00 sec)

mysql> create view unpublished as select * from node n join node_field_data nfd using (nid, vid, type, langcode) where nfd.status = 0;
Query OK, 0 rows affected (0.00 sec)

mysql> select * from unpublished;
+-----+------+---------+----------+--------------------------------------+-------+-----+--------+------------+------------+---------+--------+-------------------------------+------------------+-----------+----------+------------------------------------+
| nid | vid  | type    | langcode | uuid                                 | title | uid | status | created    | changed    | promote | sticky | revision_translation_affected | default_langcode | workspace | _deleted | _rev                               |
+-----+------+---------+----------+--------------------------------------+-------+-----+--------+------------+------------+---------+--------+-------------------------------+------------------+-----------+----------+------------------------------------+
|   2 |    2 | article | en       | fc7ff43e-ef07-4ef1-86c3-d2906446fcba | r     |   1 |      0 | 1471883667 | 1471883670 |       1 |      0 |                             1 |                1 |         1 |        0 | 1-cad41cfec6d3497384fd89be8cb51d18 |
+-----+------+---------+----------+--------------------------------------+-------+-----+--------+------------+------------+---------+--------+-------------------------------+------------------+-----------+----------+------------------------------------+
1 row in set (0.00 sec)

alexpott’s picture

Another idea might be to have a multi-column status field. The default value is the current the boolean we know and love. The other value is then something that gave have multiple values. What these values mean should have different meaning as designated by the entity type - for example Node, User and Comment all have different states. Their implementation should then maps these status back to one of the current boolean values for the old value. The implementation should also specify a default other value when the old status boolean is changed.

So taking the Node example. We have these states - unpublished/draft, published, archive

Status New_Thing Meaning
0 0 draft
0 2 unpublished*
1 1 published*
0 3 archived

We add methods to set the New_Thing and put the logic to keep them sync in the field... so if someone moves status from 1 to 0 we just move the New_Thing to 2 - the * denotes the default behaviour when setting the status and not the New_Thing.

amateescu’s picture

@alexpott, I'm not sure how much that helps with the problem described by @catch in #41, entity queries would still have to account for the new status column, just like they would for a new field.

catch’s picture

@amateescu the value of status is a valid value for new_thing always. So a query for published nodes using status = 1 continues to work.

If you need to differentiate between archived, draft, unpublished you'd have to query on new_thing, but existing queries already can't do that, so they'll work with the granularity they have currently.

This does means that admin/content would show archived nodes unless we update the query, but that seems like the least amount of updating to do.

amateescu’s picture

Right, what I'm trying to say is that having an extra column on an existing field vs. adding a new field (the original goal of this issue) is not very different in regard to the amount of things/queries that need to be updated :)

catch’s picture

@amateescu, oh that's true. The main difference in alexpott's proposal is that there'd be no automatic additional query on archived = 0 - it would have to be added explicitly by queries wanting to exclude those entities. Whether it's an extra field or an extra column on a field is secondary - it might help with validating that you can't have something status = 1 and archived = 1 at the same time etc.

timmillwood’s picture

So when we update the status, it would automatically update the new_field? and visa-versa?
I guess if there's an update setting status to 0 it would need to do a lookup on the previous new_field value to know what to set the new_field to.

timmillwood’s picture

I think the SQL views option still has some legs, so been looking into that. I created 1000 nodes with devel, then added the following view.

create view published as select * from node n join node_field_data nfd using (nid, vid, type, langcode) where nfd.status = 1;

MYSQL 5.7

mysql> explain select * from published\G;
*************************** 1. row ***************************
           id: 1
  select_type: SIMPLE
        table: n
   partitions: NULL
         type: ALL
possible_keys: PRIMARY,node__vid,node_field__type__target_id
          key: NULL
      key_len: NULL
          ref: NULL
         rows: 1000
     filtered: 100.00
        Extra: NULL
*************************** 2. row ***************************
           id: 1
  select_type: SIMPLE
        table: nfd
   partitions: NULL
         type: eq_ref
possible_keys: PRIMARY,node__id__default_langcode__langcode,node__vid,node_field__type__target_id,node__status_type
          key: PRIMARY
      key_len: 18
          ref: drupal1.n.nid,drupal1.n.langcode
         rows: 1
     filtered: 5.00
        Extra: Using where
2 rows in set, 1 warning (0.00 sec)

MYSQL 5.6

mysql> explain select * from published\G;
*************************** 1. row ***************************
           id: 1
  select_type: SIMPLE
        table: nfd
         type: ALL
possible_keys: PRIMARY,node__id__default_langcode__langcode,node__vid,node_field__type__target_id,node__status_type
          key: NULL
      key_len: NULL
          ref: NULL
         rows: 1000
        Extra: Using where
*************************** 2. row ***************************
           id: 1
  select_type: SIMPLE
        table: n
         type: eq_ref
possible_keys: PRIMARY,node__vid,node_field__type__target_id
          key: PRIMARY
      key_len: 4
          ref: drupal.nfd.nid
         rows: 1
        Extra: Using where
2 rows in set (0.00 sec)
timmillwood’s picture

After talking to @catch on IRC I'm a little unsure of the SQL Views idea. I think we all agree it's a good solution, maybe even an ideal solution, but the complexity and upheaval makes it a tough sell.

The multi-column approach is not ideal, but the upheaval is fairly small.

I will start from the patch in #47 and look to convert the StatusItem field into a multi-column field.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
8.36 KB
3.85 KB

Here's a semi-working implementation of what @alexpott suggested in #52, just wanted to get some feedback from testbot and everyone on this issue.

Status: Needs review » Needs work

The last submitted patch, 60: wi_add_statusitem-2725463-60.patch, failed testing.

timmillwood’s picture

Most if not all of the failing tests in #60 relate to views. I wasn't sure how much of the Views integration in #47 would still apply, so I removed it all. @dawehner do you mind taking a look?

This patch also changes how the database column names are generated. Currently for multi-column fields Drupal names the columns with $field_name . '__' . $property_name. Therefore the field that used to be for $entity->status->value will be names 'status__value' instead of just 'status'. The patch changes this to use just the field name if the property name is 'value'. Another option would be to keep 'status__value' and add an update hook to change all existing sites to the new column name, personally I think it'd be better to keep the existing column name.

dawehner’s picture

+++ b/core/lib/Drupal/Core/Entity/Sql/DefaultTableMapping.php
@@ -201,7 +201,12 @@ public function getFieldColumnName(FieldStorageDefinitionInterface $storage_defi
-      $column_name = count($storage_definition->getColumns()) == 1 ? $field_name : $field_name . '__' . $property_name;
+      if (count($storage_definition->getColumns()) == 1 || $property_name == 'value') {
+        $column_name = $field_name;
+      }
+      else {
+        $column_name = $field_name . '__' . $property_name;

I think this would be a huge pain not only in terms of getting stuff like views working, but actually way more important for sites. This would require a lot of updates, basically change every text with format and date field table. To be honest I don't get why we need this in the first place, because well, we have build an abstraction, so we don't have to care about the inside looking.

catch’s picture

I had the same reaction as #63. The column names aren't part of the API. Doing an update on status is nothing compared to having to do updates on any fields that have different schema as a result of the new condition. @timmillwood mentioned there could be trouble since status is an entity key, but again that feels like an easier thing to fix - either via adding multi-column support to entity keys if it doesn't already work, or making status not an entity key?

dawehner’s picture

That's weird, because I was sure we actually support 'value' in there, see (\Drupal\Core\Entity\ContentEntityBase::__construct

              if (is_array($this->values[$field_name][$langcode])) {
                if (isset($this->values[$field_name][$langcode][0]['value'])) {
                  $this->translatableEntityKeys[$key][$langcode] = $this->values[$field_name][$langcode][0]['value'];
                }
              }
              else {
                $this->translatableEntityKeys[$key][$langcode] = $this->values[$field_name][$langcode];
              }
            }
timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.26 KB
4.96 KB

Sharing an update patch:
- Adds a test to make sure the two columns have the right data when updating published or archived status
- Adds logic to update both columns when one is updated

Like EntityReference I am using onChange to keep the columns in sync, the only issue is when setting the entity as archived the value column is actually set to unpublished, then when changing the entity from archived to unpublished the onChange method doesn't get fired. So need to do some more thinking there.

This patch doesn't address the issue expressed in #63 etc.

Status: Needs review » Needs work

The last submitted patch, 66: wi_add_statusitem-2725463-66.patch, failed testing.

The last submitted patch, 66: wi_add_statusitem-2725463-66.patch, failed testing.

The last submitted patch, 66: wi_add_statusitem-2725463-66.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
11.4 KB
1.34 KB

Lets try that again.

Also fixed the archived to unpublished issue.

Status: Needs review » Needs work

The last submitted patch, 70: wi_add_statusitem-2725463-70.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
12.34 KB
1.9 KB

Resolved #63, we now have no 'status' column, just 'status__value' and 'status__state', this does create further issues, especially with views, but the patch does at least allow a site install.

Status: Needs review » Needs work

The last submitted patch, 72: wi_add_statusitem-2725463-72.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
14.35 KB
2.69 KB

Fixing a couple of the bugs

Status: Needs review » Needs work

The last submitted patch, 74: wi_add_statusitem-2725463-74.patch, failed testing.

timmillwood’s picture

Status: Needs work » Needs review
FileSize
17.59 KB
3.24 KB

Replacing all instances of n.status with n.status__value.

Status: Needs review » Needs work

The last submitted patch, 76: wi_add_statusitem-2725463-76.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
20.53 KB
2.94 KB

Maybe this fixes some stuff ... but its tricky, as well, renaming a db column causes all kind of issues ...

Status: Needs review » Needs work

The last submitted patch, 78: 2725463-78.patch, failed testing.

amateescu’s picture

It seems to me that the approach of having a multi-column field type is far more disruptive than having two separate fields. So why are not interested in going back to the initial proposal?

Also, as far as I see, #59 doesn't really mention any actual problem with the sql views approach...

timmillwood’s picture

Going back to a patch based on #70, changing column names is not going to make sense. It'll break contrib/custom queries, and core is not abstracted as first thought.

I still think a multi-column field has benefits over two fields, so following that for now.

Status: Needs review » Needs work

The last submitted patch, 81: wi_add_statusitem-2725463-81.patch, failed testing.

catch’s picture

OK I had a long think about this issue this morning, and I think we need to step back from the implementation (field vs. multiple column, exactly what queries get altered when) and look at what the use case is and what's necessary to achieve it.

As far as I can tell the main user story is:

    
As a site owner
When a content editor deletes some content
The content should not be deleted from the database
And it should be hidden from everywhere on the site except for a trashbin

However, that's been conflated a bit with CRAP. We first discussed CRAP back in 2011 or earlier, for example https://groups.drupal.org/node/133579 mentions it.

CRAP is replacing Create Read Update Delete with Create Read Archive Purge.

The 'archive' bit in this issue is supposed to line up with the A in CRAP, but I'm not sure that it actually does. The A in CRAP is replacing the U in CRUD - and this is the really important distinction.

i.e. the main change to core entity storage with CRAP would be that instead of updating an entity in-place, we always save a new revision - this is what #2696555: Remove the option to create a new revision or not from the node form moves towards. We'd never UPDATE a revision/entity in place, we CREATE a new revision and ARCHIVE the old one (implicit since it's no longer the default revision).

Then 'PURGE' means that if you didn't want to track history for an entity, you could CREATE the new revision and PURGE the old one - so there's only ever a single revision of an entity. This would look like a non-revisionable entity, except all entity saves would have a single code path, i.e. saves from the UI never result in isNewRevision being FALSE, and we potentially deprecate support in the API with the intention to remove it in 9.x.

This means CREATE is the same, READ is the same, UPDATE is CREATE + ARCHIVE (revisions). But PURGEing is just a different word for DELETE, (because CRAP is more fun to say than CRAD).

What I feel is being proposed here is really a specific workflow, and I'm not sure why it couldn't be implemented using a workflow, i.e.:

1. Add a new workflow state 'deleted'
2. Map that deleted workflow to 'unpublished' on entities (relies on introducing the concept of status to more entities, but we need that for full site preview anyway)
3. Configure admin listings to not show 'deleted' items.
4. Have a special admin page (the trash bin) that shows 'deleted' items, and allows them to be moved (configurable, but defaulting to 'draft' probably).
5. Remove all permissions to delete content
6. Optionally have some kind of trash bin emptying process that actually deletes things in the background (based on time etc.)

If we did that, we wouldn't need any special change to the entity API/schema here, just re-use the workflow and views integration for it that's being added anyway.

timmillwood’s picture

FileSize
45.75 KB

Fabianx’s picture

+1 to #83:

Status is on entities and entity revisions, but can have different states as something that is published within a workspace is not necessarily published for the site.

So you know the original status when you "archive" an entity, as it just means you unpublish it within a workspace, then publish that "change".

And to not show unpublished content by default is pretty reasonable as long as there is an easy way to get it back via the archive.

Fabianx’s picture

Discussed in IRC:

Yes, for any simple things you can simply have:

- Unpublish content instead of archiving, filter views to now show unpublished content by default

Amateescu pointed out that the main thing is that users search for "delete" instead of thinking "unpublish", so this is only suitable for small sites, where deletion in general is also okay.

For more complex things you indeed want a simple workflow and also want to track that history and be able to restore the published state, etc.:

- draft => published => archived => published (/draft - maybe)

And that way its fine to depend "trash" on content_moderation.

"archived" in this case indeed is the same as an unpublished draft.

But for the user when enabling 'trash', 'Delete' would be changed to 'Archive' and we would be doing all this workflow stuff "behind the scenes".

timmillwood’s picture

Status: Needs work » Postponed

ok, so if we're looking at this from a Product Manager point of view the end user function we're after is Trash ([#2786135#). From what @catch rightly said in #83 there is no need for a new archived field or StatusItem to get Trash working.

I have started a 8.x-2.x release of Trash module where I will put together a proof of concept depending on Content Moderation instead of Multiversion, and use the 'archived' moderation state instead of the 'archived' base field to determine if an entity should be in the trash or not.

Until we know more I am going to postpone this issue, and we could in fact postpone the whole of #2725447: WI: Phase D: Enable archive/purge storage.

timmillwood’s picture

For those looking to follow along, the Trash module is currently working with Content Moderation in #2797247: Add experimental Trash module.

Version: 8.3.x-dev » 8.4.x-dev

Drupal 8.3.0-alpha1 will be released the week of January 30, 2017, which means new developments and disruptive changes should now be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

dixon_’s picture