silverstripe / silverstripe-versioned

Versioning for Silverstripe model
BSD 3-Clause "New" or "Revised" License
8 stars 36 forks source link

Many_many relations using "through" don't unpublish after delete #116

Closed Valandur closed 6 years ago

Valandur commented 6 years ago

When using a many_many relation with through, data objects that are referenced through this relationship and removed from the draft stage and then published don't get removed from the live stage.

Following a minimal code sample to reproduce this issue, tested with SilverStripe 4.0.0, 4.0.3 and 4.1.0-rc2:

Page.php

<?php

use SilverStripe\CMS\Model\SiteTree;
use SilverStripe\Forms\GridField\GridField;
use SilverStripe\Forms\GridField\GridFieldConfig_RelationEditor;

class Page extends SiteTree {
    private static $db = [];

    private static $has_one = [];

    private static $many_many = [
        "TestObjects" => [
            "through" => TestObjectRel::class,
            "from" => "Page",
            "to" => "TestObject"
        ]
    ];

    private static $owns = [
        "TestObjects"
    ];

    public function getCMSFields() {
        $fields = parent::getCMSFields();

        $f = new GridField("TestObjects", "TestObjects", $this->TestObjects(), GridFieldConfig_RelationEditor::create());
        $fields->addFieldToTab("Root.TestObjects", $f);

        return $fields;
    }
}

TestObject.php

<?php

use SilverStripe\ORM\DataObject;

class TestObject extends DataObject {

    private static $db = [
        "Content" => "HTMLText"
    ];

    private static $belongs_many_many = [
        "Pages" => Page::class . ".TestObjects"
    ];
}

TestObjectRel.php

<?php

use SilverStripe\ORM\DataObject;

class TestObjectRel extends DataObject {

    private static $db = array(
        "RandomStuff" => "Varchar(255)",
    );

    private static $has_one = array(
        "Page" => Page::class,
        "TestObject" => TestObject::class,
    );
}

(All objects have the versioned extension applied through mysite.yml)

themes\simple\templates\Layout\Page.ss

<% include SideBar %>
<div class="content-container unit size3of4 lastUnit">
    <article>
        <h1>$Title</h1>
        <div class="content">$Content</div>
        <% loop $TestObjects %>
            <h2>$Content</h2>
        <% end_loop %>
    </article>
        $Form
        $CommentsForm
</div>

Steps to reproduce

To get the issue perform the following steps:

  1. dev/build?flush
  2. On the home page, go to the "Test Objects" tab and add a new "Test Object"
  3. Write something like "Object A" in the content, save & publish the object
  4. Go back to the page and add another object
  5. Write something like "Object B" in the content, save & publish the object
  6. Go back to the page. (At this point there should be 2 objects attached to the page in draft mode, and none in live mode)
  7. Publish the page, this should make both objects visible in the live view
  8. Remove one of the objects, by "unlinking" it
  9. Publish the page again
  10. The object that was removed is gone in draft mode, but not in live mode

Expected results:

Notes

It seems that in the database the TestObjectRel_* tables always show Version=1, even after multiple iterations of changing and publishing objects, but I don't know if that is relevant or has anything to do with this issue.

dhensby commented 6 years ago

Module versions, please

kinglozzer commented 6 years ago

Remove one of the objects, by "unlinking" it

Is this by using the “unlink” button in the GridField view? Or by clicking to edit the item and then deleting it?

If the former, I have a feeling this may be a similar issue to https://github.com/silverstripe/silverstripe-versioned/issues/78

Valandur commented 6 years ago

@dhensby for the test I did with 4.1.0-rc2 my composer.json was:

        "php": ">=5.6.0",
        "silverstripe-themes/simple": "~3.2.0",
        "composer-plugin-api": "^1.1",
        "silverstripe/recipe-plugin": "^1",
        "silverstripe/googlesitemaps": "^1.8",
        "symbiote/silverstripe-gridfieldextensions": "^3.0",
        "silverstripe/admin": "^1.1@dev",
        "silverstripe/asset-admin": "^1.1@dev",
        "silverstripe/campaign-admin": "^1.1@dev",
        "silverstripe/cms": "4.1.x@dev",
        "silverstripe/errorpage": "^1.1@dev",
        "silverstripe/graphql": "^1.1@dev",
        "silverstripe/reports": "4.1.x@dev",
        "silverstripe/siteconfig": "4.1.x@dev",
        "silverstripe/versioned": "^1.1@dev",
        "silverstripe/assets": "^1.1@dev",
        "silverstripe/config": "^1.1@dev",
        "silverstripe/framework": "4.1.x@dev"

@kinglozzer Yes it is indeed similar to that, although I'm not trying to entirely delete the object, I just want to remove it from the current page, as it may still be connected to other pages. If I understand that issue correctly one idea was to remove the button, and force the user to click on the object to delete it, which is not what I was trying to achieve, because that would remove the object everywhere where it is linked, if I understand this correctly?

kinglozzer commented 6 years ago

Yeah, I think the component you’re using is GridFieldDeleteAction, which by default will only act on the current stage (when you’re in the CMS, the current stage is draft)

Valandur commented 6 years ago

I'm sorry, but I'm a little confused. I understand that the button will only delete the relation in the draft stage, but that is also what I would expect until you publish the Page. Or did I misunderstand?

tractorcow commented 6 years ago

@Valandur what if you add this code

class Page extends SiteTree {
+        private static $has_many = [
+                'TestObjectMapping' => TestObjectRel::class,
+        ];
+        private static $cascade_deletes = [
+                'TestObjectMapping',
+        ];

This should ensure that your mapping deletion is published when you publish the parent page.

Think of cascade_deletes as a reverse owns, that unpublishes recursively instead of publishing. :)

Because a many_many through is just a has_many used to back a many_many, you can interact with this list either as a many_many or a has_many (without adding extra database schema).

I admit this isn't always the most obvious, but we could probably build this into the docs.

tractorcow commented 6 years ago

This is the code in ChangeSetItem that ensures that cascade_deletes are unpublished when publishing the parent record.

// When publishing, use cascade_deletes to partially unpublished sets
        if ($liveRecord) {
            foreach ($liveRecord->findCascadeDeletes(true) as $next) {
                /** @var Versioned|DataObject $next */
                if ($next->hasExtension(Versioned::class) && $next->hasStages() && $next->isOnLiveOnly()) {
                    $this->mergeRelatedObject($references, ArrayList::create(), $next);
                }
            }
        }

In this case the partially unpublished set is the mapping TestObjectRel

Valandur commented 6 years ago

Thank you very much for the input 👍 This seems to partly work - consider the following actions:

  1. Add a new TestObject, with content AAAA (ID 1), and publish it
  2. Add a new TestObject, with content BBBB (ID 2), and publish it
  3. Publish the page on which these test objects were created
  4. Remove TestObject AAAA (ID 1) from the page (unlink, don't delete)
  5. Publish the page
    This works properly now, the TestObject AAAA is removed from the live page
  6. Re-add the TestObject AAAA (ID 1) (using the "Link Exisiting" button, searching for ID 1)
  7. Publish the page Now the page still only shows TestObject BBBB in live mode, although TestObject AAAA was added to the page again
tractorcow commented 6 years ago

Ah that's odd. I wonder why the page isn't published? It should be. Hmm...

tractorcow commented 6 years ago

Is AAAA published? I guess that AAAA exists in TestObject_Live, but the mapping record TestObjectRel_Live doesn't have the mapping right? @Valandur

Valandur commented 6 years ago

Yes that is indeed what happens. The object itself is published in the TestObject_Live Table, but the relation record is missing in the TestObjectRelLive table.
Just as a side note: The records in the TestObjectRel
* Tables always seem to have version number 1, idk if this is by design.

tractorcow commented 6 years ago

Do you still have

private static $owns = [
        "TestObjects"
    ];

That SHOULD publish not only the objects in the many_many, but also in the mapping table.

Valandur commented 6 years ago

Apologies for the late reply, but yes, I still have the $owns relation. I can set up a minimal repo for you if that would help.

chillu commented 6 years ago

@tractorcow Can you please make sure the cascade_deletes is pointed out where we explain versioned "through" relationship in the docs?

tractorcow commented 6 years ago

Sure.

mfendeksilverstripe commented 6 years ago

I can confirm that many_many_through seems to ignore the current versioned stage. Here is my code example (code is called within context of the Page):

// we have one record in draft stage
$draftCount = $this->TestObjectMapping()->count();

// switch to live mode
$origReadingMode = Versioned::get_reading_mode();
Versioned::set_reading_mode(Versioned::LIVE);

// we have no records in live stage
$liveCount = $this->TestObjectMapping()->count();

// witch back to original reading mode
Versioned::set_reading_mode($origReadingMode);

// $draftCount contains 1
// $liveCount contains 1 and should contain 0

$liveCount = Versioned::get_by_stage(
    TestObjectRel::class,
    Versioned::LIVE,
    ['ParentID' => $this->ID]
)->count();

// $liveCount contains 0 which is correct

The same behavior can be viewed on frontend when ManyManyThroughList is being used. It always shows the DRAFT stage.

We're currently on this commit for Versioned module:

5db190cc8f722cc59593b47590c1e027b1961128

Btw this issue should be probably renamed because the original title indicates an issue that was resolved.

tractorcow commented 6 years ago

I can confirm that many_many_through seems to ignore the current versioned stage. Here is my code example (code is called within context of the Page):

Yes this is intentional. This should respect the stage of the parent record, not the current global state. Versioned::set_reading_mode($origReadingMode); only sets the reading mode for new queries.

mfendeksilverstripe commented 6 years ago

@tractorcow So how do I actually get to show LIVE version of the relationship data on the frontend (when viewing LIVE stage)? It's currently showing DRAFT data at all times. The stage of the parent is LIVE in that case, but I'm still getting DRAFT relationship data.

tractorcow commented 6 years ago

Get the live version of the page and call ->TestObjectMapping on it().

$record = Versioned::get_by_stage(TestObject::class, Versioned::LIVE)->byID($recordID);
$record->TestObjectMapping(); // live records
$draft = Versioned::get_by_stage(TestObject::class, Versioned::DRAFT)->byID($recordID);
$draft->TestObjectMapping(); // draft records
tractorcow commented 6 years ago

Just to add some justification to this, the intention was to remove the need to constantly modify global state; This often left websites in inconsistent states due to constants set / do something / restore state operations.

You can also use Versioned::withVersionedMode if you absolutely need to make temporary changes to the global state, but in most cases you do not.

mfendeksilverstripe commented 6 years ago

Thanks for the clarification. It seems to be working as intended :) I guess this issue can be closed then?

tractorcow commented 6 years ago

I'm still a bit concerned about @Valandur 's comment at https://github.com/silverstripe/silverstripe-versioned/issues/116#issuecomment-367651423

Publish the page Now the page still only shows TestObject BBBB in live mode, although TestObject AAAA was added to the page again

Is this still broken, given anything I've explained up until now?

mfendeksilverstripe commented 6 years ago

This case seems to be working for me, but I have slightly different setup. Instead of Page, I have the relationship on a Block. Instead of using the default AddedExistingAutocomple We have our own custom GridField component to add relation data but internally it should be doing the same:

$list->add($item);

add() is available both on ManyManyList and ManyManyThroughList

tractorcow commented 6 years ago

I think any existing issues are probably unrelated to the original issue; Resolved by adding owns and cascade_deletes.