Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inconsistency in versioning system #5136

Open
kubaplas opened this issue Oct 23, 2019 · 14 comments

Comments

@kubaplas
Copy link
Contributor

@kubaplas kubaplas commented Oct 23, 2019

Bug Report

-- continuing discussion from gitter --
In Pimcore 6, unpublished object data is not stored in the database - only in the versions. This cause some confusion, as the data is not accessible for grid and PHP API - until it is published.

See also: https://talk.pimcore.org/t/save-button-only-creates-new-version/2727

Steps to reproduce

  1. Create a new object in admin panel.
  2. Fill in the data, save the object using "Save" button
  3. Display object in the grid - data is not available here.
  4. Data is searchable, but not visible in search grid

Code example

// object created in admin panel, attrbute set and object saved, but not published
$objList = new Listing();
$objList->setCondition("attribute = ?", ["uniquevalue"]);
$objList->setUnpublished(true);
echo ($objList->count()); // echoes "0"

// pure PHP API
$obj = new MyObject();
$obj->setKey("myKey");
$obj->setParent(Folder::getByPath("/"));
$obj->setAttribute("uniquename");
$obj->save();

$objList = new MyObject\Listing();
$objList->setCondition("attribute = ?", ["uniquename"]);
$objList->setUnpublished(true);
echo ($objList->count()); // echoes 1 - this time it works

Proposed solutions

  1. Revert to the behaviour from Pimcore 5 - data is stored in the database until object is published
  2. Make the behaviour consistent everywhere
@dpfaffenbauer

This comment has been minimized.

Copy link
Contributor

@dpfaffenbauer dpfaffenbauer commented Oct 23, 2019

  1. Are you sure with 1? I can't imagine it changed from 5 to 6.
@kubaplas

This comment has been minimized.

Copy link
Contributor Author

@kubaplas kubaplas commented Oct 24, 2019

@dpfaffenbauer Just tested it on Pimcore 5.6.2 - I created an object via admin panel, set the data and saved. Data is in the db and visible on grid.

@mkrych

This comment has been minimized.

Copy link

@mkrych mkrych commented Oct 24, 2019

@kubaplas Actually it changed with version 6.2.0 (6.1v worked ok).
Issue: #4905

@dpfaffenbauer

This comment has been minimized.

Copy link
Contributor

@dpfaffenbauer dpfaffenbauer commented Oct 24, 2019

Interesting, might also explains issues with CoreShop: coreshop/CoreShop#1118

@fashxp

This comment has been minimized.

Copy link
Member

@fashxp fashxp commented Oct 29, 2019

The thing is ... via admin saveVersion() is called, see:

Via API you most probably call save().

Before #4905 a setPublished(false) and save() was called in admin - which is also completely wrong.

So completely restore old behavior ( 1) ) is not an option.
Right now, it is quite consistent, but users might expect different behavior from the save button.

What about following definition?

  • if element published -> save button (which is visible to users with save but without publish permission only) calls saveVersion() and does not update DB/grid data
  • if element not published -> save button (which is visible to all users with at least save permission) calls save() and does update DB/grid data
@kubaplas

This comment has been minimized.

Copy link
Contributor Author

@kubaplas kubaplas commented Oct 30, 2019

@fashxp Your proposal sounds good. Actually that's what I meant by Pimcore 5 behaviour ;) (I was not aware of the unpublishing thing).

Maybe in first case the button should be labeled just "Save version"?

@mkrych

This comment has been minimized.

Copy link

@mkrych mkrych commented Oct 31, 2019

The thing is ... via admin saveVersion() is called, see:

  pimcore/bundles/AdminBundle/Controller/Admin/DataObject/DataObjectController.php


     Line 1312
  in
  b82d45b





    
      
                   $object->saveVersion(); 

Via API you most probably call save().
Before #4905 a setPublished(false) and save() was called in admin - which is also completely wrong.
So completely restore old behavior ( 1) ) is not an option.
Right now, it is quite consistent, but users might expect different behavior from the save button.
What about following definition?

if element published -> save button (which is visible to users with save but without publish permission only) calls saveVersion() and does not update DB/grid data
if element not published -> save button (which is visible to all users with at least save permission) calls save() and does update DB/grid data

That's how it worked till version 6.2.0
What is more button "Save and exit" still calls save() (so it works diffrent than save button). Also changing attribute value in grid view also does update DB.

@fashxp

This comment has been minimized.

Copy link
Member

@fashxp fashxp commented Oct 31, 2019

@mkrych no it didn't, before 6.2.0 hitting save-button unpublished the object.
but yes, save&close is also not consistent.

@mkrych

This comment has been minimized.

Copy link

@mkrych mkrych commented Oct 31, 2019

@fashxp
I see, so maybe user that doesn't have publish permission should see only "Save only new version" button (on published object). If object is published there should be no "Save" option, only "Save&Publish" or "Save only new version". "Save" button should be only available if element is not published.
I think this way it would be more intuitive for the user.

@kubaplas

This comment has been minimized.

Copy link
Contributor Author

@kubaplas kubaplas commented Nov 13, 2019

@fashxp any thoughts about how we should address this?

@pivaker

This comment has been minimized.

Copy link

@pivaker pivaker commented Nov 20, 2019

@fashxp @kubaplas
Are there any plans to change this behaviour?
Right now our PIM editors are not happy because they want to make changes (modify objects) that is planned to be published after a week, they don't want to make the changes on the same day that they will go live (too many objects). And if they cant use the grid view to bulk publish the changes they have to open every single object by object. I know about the Schedule feature in Pimcore but that doesn't fit our needs.
In my opinion the bulk editing in grid view must consider versions (changes) of the objects, this seems like a basic thing in a PIM system.

@aarongerig

This comment has been minimized.

Copy link
Contributor

@aarongerig aarongerig commented Nov 20, 2019

☝🏻 and also inheritance, which doesn't seem to work with unpublished objects (version only) either!

@dpfaffenbauer

This comment has been minimized.

Copy link
Contributor

@dpfaffenbauer dpfaffenbauer commented Nov 20, 2019

Honestly, Pimcore has a huge inconsistency in it's core when it comes to Version. When I work in the the Backend, I expect to ALWAYS see the latest Version, as DataObject Detail, or in Grids. When I am in the frontend, I wan't to see the latest published version.

But: How to query for the latest published version using Listings when the latest Version is stored in the Database.

But: How to query to the latest Version using Listings (Pimcore Backend) when the latest published Version is store din the Database.

So that is never gonna work or satisfy every use-case. In a perfect world, the Versioned Information should also be queryable, but that means having either a version table for every DataObject, or a version_id in every DataObject Table, that might create HUGE Data and make certain tables slower.

So, what is the actual Point-of-View on this matter from the Pimcore Team?

@fashxp

This comment has been minimized.

Copy link
Member

@fashxp fashxp commented Nov 20, 2019

we have to discuss that internally ... as you also already notice, this is a very complex topic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.