U4-1363 - Setting properties on Document doesn't work with old API - UmbracoCms.6.0.0.build.93

Created by Jeroen Breuer 28 Dec 2012, 21:50:27 Updated by Sebastiaan Janssen 15 Jan 2013, 13:05:53

Is duplicated by: U4-1362

Relates to: U4-1401

When you install the [DAMP 2.0 Samples|http://our.umbraco.org/FileDownload?id=3145] package it will set some properties on a Document with this code:

//Save the media items in the node.

int nodeId = GetIdByName(DAMP_NODE_NAME); Document document = new Document(nodeId);

string[] fileIds = new string[] { file1.Id.ToString() }; string[] wideIds = new string[] { imageWide1.Id.ToString(), imageWide2.Id.ToString(), imageWide3.Id.ToString() }; string[] longIds = new string[] { imageLong1.Id.ToString(), imageLong2.Id.ToString() };

document.getProperty("dampFile").Value = DigibizMediaHelper.GetXML(fileIds).ToString(); document.getProperty("dampClassic").Value = image1.Id.ToString(); document.getProperty("dampNewWide").Value = DigibizMediaHelper.GetXML(wideIds).ToString(); document.getProperty("dampNewLong").Value = DigibizMediaHelper.GetXML(longIds).ToString();

document.Publish(adminUser); umbraco.library.UpdateDocumentCache(document.Id);

This doesn't give any errors, but the properties aren't set and the Document isn't published.

Comments

Morten Christensen 28 Dec 2012, 22:00:15

I believe it works as its been tested, but from the code you have provided you are missing a document.Save(). The Document class has been refactored to internally use the new api, which means that the value on a property isn't saved directly to the database, but rather as part of a save operation (and transaction).


Jeroen Breuer 28 Dec 2012, 22:18:46

Just added document.Save() and now it works :-). However this is probably a pretty big breaking change. Even the [official API cheatsheet|http://our.umbraco.org/wiki/reference/api-cheatsheet/modifying-document-properties] which is used during the level 2 course doesn't have the .Save() call. The Document API is probably the most used API in Umbraco and will be used in a lot of websites so I think this is pretty important.

Is it possible that if .Save() is not called that the document also isn't published?


Sebastiaan Janssen 29 Dec 2012, 09:22:05

@Morten can we make it so that inside Publish, the Save() happens first? Maybe make an overload (bool NoSave) to help people that do want to call Save() themselves? We've been discouraging people trying to use Save() as it didn't actually do anything before.


Morten Christensen 29 Dec 2012, 10:01:44

I'm not sure if thats a better idea, because you'd just be double saving when people are actually using the .Save() method. And even though it might not have been necessary in the past the most logically thing would be to call save when you want to save ;) Also, wouldn't it be better that people start using .Save in transitioning to the new api where you need to call seve in order to save? But I agree with Jeroen that we should make this change more apparent and explain why its now necessary to call save ... when wanting to save :)


Morten Christensen 29 Dec 2012, 10:08:41

And just for the record, we could call SaveAndPublish (in the new api) in the Publish method on the legacy Document class, but it would trigger an unnecessary amount of save operations, which is just as hacky as not calling save IMO.


Jeroen Breuer 29 Dec 2012, 10:13:28

The old API is hacky anyway ;-) so if calling SaveAndPublish doesn't break things like the [DAMP 2.0 Samples|http://our.umbraco.org/FileDownload?id=3145] package I think I prefer that. Do things right in the new API and just try not to break the old one :-).


Morten Christensen 29 Dec 2012, 10:53:56

Sure, but no matter which route we take there are gonna be people who have to adapt, so I'd prefer to take the path that makes the most sense and less hacky ;)

Consider this: When saving a document you'd have to do this: Document document = new Document(nodeId); document.getProperty("dampClassic").Value = image1.Id.ToString(); document.Save();

To Save and Publish a document (with saving in the Publish method): Document document = new Document(nodeId); document.getProperty("dampClassic").Value = image1.Id.ToString(); document.Publish(adminUser); umbraco.library.UpdateDocumentCache(document.Id); -- You'd need to note that if you were already calling .Save() you'd trigger an unnecessary save operation.

To Publish without saving: Document document = new Document(nodeId); var result = document.PublishWithResults(adminUser); umbraco.library.UpdateDocumentCache(document.Id);

Publishing with children doesn't trigger any save operation: PublishWithChildrenWithResult

Now, what annoys me with the above is that the Publish method doesn't indicate that the Document will get saved if we added saving to that method. I'd prefer to keep the methods as they are and instead introduce a SaveAndPublish method in the Document class. I think that is way more obvious and less intrusive, no? We'd still have to update the documentation on saving a Document, but hopefully the changes will be logically enough to make sense.

Let me know what you think.


Scott Williams 30 Dec 2012, 19:55:52

I would introduce '''SaveAndPublish''' but in the process obsolete '''Publish''' method and just have '''Publish''' call '''SaveAndPublish'''.

It shouldn't be possible to publish a local document with unsaved changes without it first being saved, you will never want to do that, could make it more efficient by tracking the dirty state of the document and only calling save internally if it is.

You need to consider that I have just been tweaking a Document, if I then call Publish then obviously I want the current version, with my new changes published, if that means it has to be saved to do that then the API should make that decision and do it, I've said publish it I want the new local version with all its changes published not some old version in the DB that happens to have the same ID.


Sebastiaan Janssen 31 Dec 2012, 10:18:32

I agree with Scott, I can think of no scenario where you'd want to publish without saving. If that leads to a few more unnecessary saves because people have used the Save method in their custom code, then so be it.


Morten Christensen 31 Dec 2012, 10:56:20

A scenario could be getting documents with a release date that ''just'' needs to be published. I don't have a problem with having the Publish method do a SaveAndPublish, I just don't like that we are changing the behaviour of yet another (legacy) method. We have already changed how Save (must) be used, and I don't see any particular need to change the Publish method as well. But thats just my personal opinion, so lets just do as suggested ;)


Sebastiaan Janssen 31 Dec 2012, 11:03:24

I understand your scenario but it's still doesn't really matter if there's an extra Save performed right before the publish. I think by adding an extra save during publish, we'll stay much closer to the behaviour that people are familiar with and we'll break way less code of people who are used to each property getting saved as soon as you assign a value to it.

We should list it as a breaking change though, to make it absolutely clear to everyone that the behavior has changed. I'll create some issues for this and updating properties.


Jeroen Breuer 31 Dec 2012, 11:09:22

I agree with Sebastiaan. Make sure the code keeps working in almost the same way is the best. That's what people are used to and probably won't cause a lot of trouble with upgrading from v4 to v6.


Jeroen Breuer 31 Dec 2012, 11:15:43

Only problem is that this would solve it for the Document API, but the Media API doesn't have a publish option so it could still break a lot of things if .Save() isn't called on that. This also goes for other things that don't call the .Save() method for example updating [Media Types|http://issues.umbraco.org/issue/U4-1362#comment=67-4375]. This is probably the biggest breaking change with the new API.


Morten Christensen 31 Dec 2012, 11:58:39

Yes, thats true but its really the only way we could refactor the legacy API without making it worse then it already is. We have to move forward and these breaking changes is the least painful way to do it. So yes Save on DocumentType, MediaType and Media (and Document as well of course) are breaking changes, but if we were to save each time a property is updated we'd be making it worse IMO, and in that case it wouldn't make sense to actually do the refactoring.

Also, not "Saving" your entities is a bad practice and the sooner we can get away from it the better ;)


Morten Christensen 03 Jan 2013, 10:45:16

I updated the Document class, so the Publish method now calls the new SaveAndPublish method. This should ensure that only calling Publish will also trigger a Save of the Document that is being published as has been discussed above.


Priority: Normal

Type: Bug

State: Fixed

Assignee: Morten Christensen

Difficulty: Normal

Category: Architecture

Backwards Compatible: True

Fix Submitted:

Affected versions: 6.0.0

Due in version:

Sprint:

Story Points:

Cycle: