Razor in 4.7.1 - Breaking changes, inconsistencies and bugs
First, let me say I love Razor in Umbraco - it is awesome and it's virtually replaced XSLT now as my language of choice in Umbraco.
However, today I stumbled into many problems when upgrading a site from 4.7 to 4.7.1 and this threw into light many inconsitencies and bugs within Razor, right now. I'm starting to wonder if Razor is suitable for use in production as it seems to be changing so fast?
In my 4.7 site I was doing something like this to display an image gallery from a folder (selected using a content picker):
dynamic folder = new DynamicMedia(1234);
foreach (var image in folder.Children)
{
<img src="@image.umbracoFile" alt="@image.Name"
width="@image.UmbracoWidth" height="@image.UmbracoHeight" />
}
This was quite neat, I thought.
However, in Umbraco 4.7.1 there is a breaking change (that isn't mentioned in the release notes!) that has changed the constructor for DynamicMedia. It no longer takes an Id you have to construct it like so:
dynamic folder = new DynamicMedia(new DynamicBackingItem(1234, DynamicBackingItemType.Media));
A bit of a mouthful, you'll agree, but it works... But, as Sebastiaan Janssen (Cultiv) pointed out to me, I should be using:
dynamic folder = Model.GetMediaById(1234);
OK, this works. But it requires a reference to @Model, so won't work in Razor helpers or other code outside the macro, which is a bit of a pain.
But looking into it I discover that Model.GetMediaById() returns a type of umbraco.MacroEngines.DynamicNode. DynamicMedia is just a wrapper around DynamicNode it turns out.
Plus, I also found in the source for DynamicNode a reference that indicated that indicates that Model.GetMediaById() is deprecated and that I should be using Library.GetMediaByID() instead. What? More confusion....
But, I then find out, that DynamicNode doesn't have a property called Children so you CAN'T do this any more:
foreach (var image in folder.Children) { }
So, I then tried to do this:
foreach (var image in folder.Descendants()) { }
This, however, actually killed Umbraco by causing a stackoverflow which shutsdown the App Pool in IIS and takes down the entire site! I've posted the bug on CodePlex.
So, not very good. I've wasted half-a-day on this...
So if I can't use Descendants() then what other methods does DynamicNode offer? Well, if you look there are two more properties:
GetChildrenAsList which returns a DynamicNodeList and the similary named ChildrenAsList which returns a List<DynamicBackingItem>.
My first thought is - why are there two very similary named properties? And why are they properties and not methods? Surely GetChildrenAsList is the name of a method and not a property? Wouldn't it have been more consistent to just call this Children if it's a property? Plus there is also a method called ChildrenAsTable() as well!
Confused? You will be...
As I say, I love Razor and really appreciate all the work of the talented devs who brought it to fruition. But it really could do with a little more... care.
I was also wondering what DynamicBackingItemType is - I have a feeling that it is a result of trying to unify DynamicNode so that it deals with both page nodes and media nodes. My guess is that DynamicBackingItemType tells the DynamicNode what type it actually is. From what I gather DynamicMedia is really just DynamicNode - it inherts from it without actually extending it. But some documenation on all this would be handy - Razor needs and deserves more than just a few blog posts (many of which are now out-of-date and provide wrong information). It's not something that the community can just do - it needs the developers to provide the basic information to help us.
@Dan: i guess you are right about the porpose of the object DynamicBackingItemType.
secondly. the guys doing a lot of work and razor is developing so fast.. and anyway i upgrade my macroengine dll to 4.7.1 on my responsibility. so, i completely understand bugs or the lack of documentation, still - breaking chnages like this should be clearly announced.
@sebastian: it will be great to here about DynamicBackingItemType if you have some info to add.
As you are probably aware, the Razor DynamicNode stuff is my area in Umbraco 4.7, currently.
For the most part, new features, bug fixes, improvements etc are sourced either from community requests on Twitter, or my own needs while templating sites.
I am still actively building CMS sites and almost 100% using razor.
With the long period of time between 4.7.0 and 4.7.1, A lot of features were added.
I tried extremely hard to maintain backwards compatibility, but as I was building sites with an edge DLL most of the time, it's entirely possible I missed something.
Hopefully I can try and address your concerns outlined in this thread, I'll do this from the top down.
@Dan:
Most of the problems that I can immediately see in this thread is due to the refactoring I did on Media.
Prior to 4.7.1, there were two seperate implementations in RazorME [MacroEngine], one for Nodes and another for Media
The reason this was done is because Media isn't backed by the NodeFactory cache, and the Razor engine used new Media to retreive the media items. This hit the database a lot and on media heavy sites could start to cause a performance issue.
Some of the features requests were to be able to call methods that were implemented on DynamicNode on DynamicMedia.
I refactored the media implementation (DynamicMedia) into one that is backed by Examine, and then to allow all of these [the DynamicNode] methods to work on media, I created the concept of a DynamicBackingItem (which has a type)
This internal class provides a standard API (INode-like) which the DynamicNode can call into, irrespective of if the source data is from a Media item or a Node.
DynamicBackingItem has two internal members, an INode (content) or a Media item, which is type ExamineBackedMedia. This media specific class talks to examine, but will also fall back to the umbraco.library.GetMedia method if the media item isn't in examine or the examine cache is broken for some reason.
So, to answer the question "What is DynamicBackingItemType?" It is an Enumeration that tells backing item what type the node is.
In one of the pre-release versions, the method Model.NodeById had a bug which meant that it didn't correctly identify Media vs Nodes, and treated everything as nodes. I added the overload which takes the type to work around this, and never removed it because I thought it might be useful to someone.
Model.NodeById (@Library.NodeById - more blog posts to come about this, there's a lot to write about and information overload isn't good) should identify node vs media and return a DynamicNode containing that item.
DynamicMedia's own implementations were removed, and I converted the class to be a simple subclass for DynamicNode.
I've just checked the code and yes, you're correct - I accidentally removed the overloads during my refactoring. I've added them back in and commited the fix. I will make an updated macroengine build available at the end of this post.
With regards to the .Children method being missing on DynamicMedia, this isn't correct, it's implemented on DynamicNode, but if you have a statically typed DynamicMedia, then there's a good chance that it won't find and call the method correctly.
Yes, DynamicMedia is just a wrapper around DynamicNode - however I checked methods still worked and I have been using .Children today on a build with the 4.7.1 release and it was working.
This works for me;
dynamic folder = Model.MediaById(1057);
foreach (var image in folder.Children)
As long as 1057 is a folder, not an item (otherwise you get an empty enumeration)
Moving on with your post,
foreach (var image in folder.Descendants()) { }
Yes - this causes a stack overflow for me, but only when you call Children on a node which doesn't have children.
Unfortunately, this is due to an oversight of mine, when DynamicBackingItem failed to find the item in examine, it fell back to GetMedia and enumerated the properties of the media item as if they were nodes.
Unfortunately, descendants calls .Children on everything via Map so this bug will bite a few people.
I have found and fixed the issue in the updated release below.
I'll ask Niels if we can get a patch release out quickly.
Moving on,
I'm not totally happy about the confusion between GetChildrenAsList and ChildrenAsList (and then Children)
One of the methods should have been marked as Private/Internal and the other method (and ChildrenAsTable) are coming from the fact that the INode interface is used, which requires these methods to be defined.
.Children is meant to return a DynamicNodeList, so you can enumerate on it and call the dynamic methods, but internally, I need a statically typed (non dynamic method) to get the real children items.
The ChildrenAsTable and GetChildrenAsList are INode methods that have been there for a while. I don't want to remove them as they'll break backwards compatibility, assuming the interface is not there anymore.
I appreciate that it's frustrating when you run up against bugs, I take a great deal of care to ensure I don't break anything and all the features I release I've been testing on my own production sites.
I commit regularly to allow people to test nightlys, but there are only a handful of people testing and due to the structure of umbraco 4, it's quite hard to develop unit tests for features such as the macro engine.
Next post..
@Eran/@Dan
You're correct, it's to do with DynamicMedia and DynamicNode being the same thing
@Mikkel
Yes that should be how you call it, the DynamicMedia is only there for backwards compatibility
I have fixed an issue with HasValue (not sure I've blogged about that one yet), the stack overflow with Descendants and have added the missing overloads back in.
I haven't yet updated our development site to 4.7.1, After seeing these I have concerns to update to 4.7.1.
Ideally, there should be additional methods or properties to handle things rather than changing the existing ones which can cause lots of problem for updating and maintaining a production site where there are lots of razor scripts already written with old methods.
Updating the site shouldn't break any existing code.
@Rajeev: If you have a dev site, it's pretty straight forward to test out the changes. We've done our best in testing the changes made to the core and we've also had nightlies and betas out. We've announced the beta and that's a great way to help testing and finding edge-case bugs, so that once the final release is outit'll be as stable as possible. That's all we can do, really. Mistakes will happen, but the more community people that helps with testing the more stable the releases will be. If everybody leaned back and waited for someone else to try, nothing would happen.
That said, when's the last time you've seen a 12 hour turnaround from reported issue to a fixed patch reading for testing. I think Gareth has done an amazing and more-than-you-could-ask-for job.
@Gareth - Many thanks for taking the time to post such a detailed and informative response. This is why I love the Umbraco community - you mention a problem and not only do you get an amazingly detailed response from one of the developers but you also get a working patch in less than 24 hours and updated documentation. Totally amazing!
Your logic in refactoring DynamicMedia so that it hangs off DynamicNode makes total sense. In fact, ever since I've been using Umbraco I've always thought they should be treated the same way as their are so many similarities. So well done for doing what Einstein could never manage and coming up with a Grand Unification of Umbraco :D
I was interested in this part:
This media specific class talks to examine, but will also fall back to the umbraco.library.GetMedia method if the media item isn't in examine or the examine cache is broken for some reason.
I didn't even realise Media items are cached using Examine?! That is really interesting. I presume this is to work around the database hits that used to occur with traditional GetMedia() calls? I'd be interested in more info on this!
Anyway, I guess my major gripe was thinking that DynamicMedia had been majorly changed without the release notes mentioning it. But I realise this wasn't deliberate and was just an oversite, which is totally fair enough - we're all human. It's just that I love Razor so much I don't want it to go the way of previous things like LinqToUmbraco and end up unusable due to changes, lack of support etc. Sorry if I sounded like I was bitching - I had a bit of a bad day and needed to vent :)
With regards to the .Children method being missing on DynamicMedia, this isn't correct, it's implemented on DynamicNode, but if you have a statically typed DynamicMedia, then there's a good chance that it won't find and call the method correctly.
OK, you are right, of course - I was looking at the methods of the statically typed DynamicMedia/Node (as that seemed to be the best way for me to discover what properties/methods are available). Once typed to dynamic Children worked as expected. I have to admit I don't really understand why this happens, but dynamic types are still a bit mysterious to me!
As for the similary named methods - I guess this is more of a problem because intellisense exposes methods and curious people (like me!) discover it. Perhaps in Umbraco 5 you can tidy things up without too much of a worry about breaking backward compatability.
OK, I'm off to play around with all the new features! Once again, thanks to Gareth and the rest of the team - brilliant work that once again surpasses my expectations.
You indicate in your post from 10 months ago the following:
DynamicBackingItem has two internal members, an INode (content) or a Media item, which is type ExamineBackedMedia. This media specific class talks to examine, but will also fall back to the umbraco.library.GetMedia method if the media item isn't in examine or the examine cache is broken for some reason.
My question is: what is the easiest way to force a particular Media item to load directly from the database, i.e., how to bypass this behavior of "GetMedia" first looking to Examine? I've tried messing with various settings in my .config files but haven't had any success.
The reason I ask is that I am trying to track down a bug and after weeks of reading through Umbraco source code, my working theory is that the bug lies in UmbracoExamine, but without recompiling source code I don't have any way to test my theory.
Razor in 4.7.1 - Breaking changes, inconsistencies and bugs
First, let me say I love Razor in Umbraco - it is awesome and it's virtually replaced XSLT now as my language of choice in Umbraco.
However, today I stumbled into many problems when upgrading a site from 4.7 to 4.7.1 and this threw into light many inconsitencies and bugs within Razor, right now. I'm starting to wonder if Razor is suitable for use in production as it seems to be changing so fast?
In my 4.7 site I was doing something like this to display an image gallery from a folder (selected using a content picker):
dynamic folder = new DynamicMedia(1234); foreach (var image in folder.Children) { <img src="@image.umbracoFile" alt="@image.Name" width="@image.UmbracoWidth" height="@image.UmbracoHeight" /> }This was quite neat, I thought.
However, in Umbraco 4.7.1 there is a breaking change (that isn't mentioned in the release notes!) that has changed the constructor for DynamicMedia. It no longer takes an Id you have to construct it like so:
A bit of a mouthful, you'll agree, but it works... But, as Sebastiaan Janssen (Cultiv) pointed out to me, I should be using:
OK, this works. But it requires a reference to @Model, so won't work in Razor helpers or other code outside the macro, which is a bit of a pain.
But looking into it I discover that Model.GetMediaById() returns a type of umbraco.MacroEngines.DynamicNode. DynamicMedia is just a wrapper around DynamicNode it turns out.
Plus, I also found in the source for DynamicNode a reference that indicated that indicates that Model.GetMediaById() is deprecated and that I should be using Library.GetMediaByID() instead. What? More confusion....
But, I then find out, that DynamicNode doesn't have a property called Children so you CAN'T do this any more:
foreach (var image in folder.Children) { }So, I then tried to do this:
foreach (var image in folder.Descendants()) { }This, however, actually killed Umbraco by causing a stackoverflow which shutsdown the App Pool in IIS and takes down the entire site! I've posted the bug on CodePlex.
So, not very good. I've wasted half-a-day on this...
So if I can't use Descendants() then what other methods does DynamicNode offer? Well, if you look there are two more properties:
GetChildrenAsList which returns a DynamicNodeList and the similary named ChildrenAsList which returns a List<DynamicBackingItem>.
My first thought is - why are there two very similary named properties? And why are they properties and not methods? Surely GetChildrenAsList is the name of a method and not a property? Wouldn't it have been more consistent to just call this Children if it's a property? Plus there is also a method called ChildrenAsTable() as well!
Confused? You will be...
As I say, I love Razor and really appreciate all the work of the talented devs who brought it to fruition. But it really could do with a little more... care.
just run into the same problem with the new constructor of DynamicMedia, after upgrading the MacroEngine dll.
i try to examine all the breaking changes before, but it seems that i miss that info too.
anyway, what exactly is this object: DynamicBackingItemType?
I was also wondering what DynamicBackingItemType is - I have a feeling that it is a result of trying to unify DynamicNode so that it deals with both page nodes and media nodes. My guess is that DynamicBackingItemType tells the DynamicNode what type it actually is. From what I gather DynamicMedia is really just DynamicNode - it inherts from it without actually extending it. But some documenation on all this would be handy - Razor needs and deserves more than just a few blog posts (many of which are now out-of-date and provide wrong information). It's not something that the community can just do - it needs the developers to provide the basic information to help us.
So instead of
We should use
@Mikkel - this code is actually working?
@Dan: i guess you are right about the porpose of the object DynamicBackingItemType.
secondly. the guys doing a lot of work and razor is developing so fast.. and anyway i upgrade my macroengine dll to 4.7.1 on my responsibility. so, i completely understand bugs or the lack of documentation, still - breaking chnages like this should be clearly announced.
@sebastian: it will be great to here about DynamicBackingItemType if you have some info to add.
Hi Guys
As you are probably aware, the Razor DynamicNode stuff is my area in Umbraco 4.7, currently.
For the most part, new features, bug fixes, improvements etc are sourced either from community requests on Twitter, or my own needs while templating sites.
I am still actively building CMS sites and almost 100% using razor.
With the long period of time between 4.7.0 and 4.7.1, A lot of features were added.
I tried extremely hard to maintain backwards compatibility, but as I was building sites with an edge DLL most of the time, it's entirely possible I missed something.
Hopefully I can try and address your concerns outlined in this thread, I'll do this from the top down.
@Dan:
Most of the problems that I can immediately see in this thread is due to the refactoring I did on Media.
Prior to 4.7.1, there were two seperate implementations in RazorME [MacroEngine], one for Nodes and another for Media
The reason this was done is because Media isn't backed by the NodeFactory cache, and the Razor engine used new Media to retreive the media items. This hit the database a lot and on media heavy sites could start to cause a performance issue.
Some of the features requests were to be able to call methods that were implemented on DynamicNode on DynamicMedia.
I refactored the media implementation (DynamicMedia) into one that is backed by Examine, and then to allow all of these [the DynamicNode] methods to work on media, I created the concept of a DynamicBackingItem (which has a type)
This internal class provides a standard API (INode-like) which the DynamicNode can call into, irrespective of if the source data is from a Media item or a Node.
DynamicBackingItem has two internal members, an INode (content) or a Media item, which is type ExamineBackedMedia. This media specific class talks to examine, but will also fall back to the umbraco.library.GetMedia method if the media item isn't in examine or the examine cache is broken for some reason.
So, to answer the question "What is DynamicBackingItemType?" It is an Enumeration that tells backing item what type the node is.
In one of the pre-release versions, the method Model.NodeById had a bug which meant that it didn't correctly identify Media vs Nodes, and treated everything as nodes. I added the overload which takes the type to work around this, and never removed it because I thought it might be useful to someone.
Model.NodeById (@Library.NodeById - more blog posts to come about this, there's a lot to write about and information overload isn't good) should identify node vs media and return a DynamicNode containing that item.
DynamicMedia's own implementations were removed, and I converted the class to be a simple subclass for DynamicNode.
I've just checked the code and yes, you're correct - I accidentally removed the overloads during my refactoring. I've added them back in and commited the fix. I will make an updated macroengine build available at the end of this post.
With regards to the .Children method being missing on DynamicMedia, this isn't correct, it's implemented on DynamicNode, but if you have a statically typed DynamicMedia, then there's a good chance that it won't find and call the method correctly.
Yes, DynamicMedia is just a wrapper around DynamicNode - however I checked methods still worked and I have been using .Children today on a build with the 4.7.1 release and it was working.
This works for me;
dynamic folder = Model.MediaById(1057);
foreach (var image in folder.Children)
As long as 1057 is a folder, not an item (otherwise you get an empty enumeration)
Moving on with your post,
foreach (var image in folder.Descendants()) { }
Yes - this causes a stack overflow for me, but only when you call Children on a node which doesn't have children.
Unfortunately, this is due to an oversight of mine, when DynamicBackingItem failed to find the item in examine, it fell back to GetMedia and enumerated the properties of the media item as if they were nodes.
Unfortunately, descendants calls .Children on everything via Map so this bug will bite a few people.
I have found and fixed the issue in the updated release below.
I'll ask Niels if we can get a patch release out quickly.
Moving on,
I'm not totally happy about the confusion between GetChildrenAsList and ChildrenAsList (and then Children)
One of the methods should have been marked as Private/Internal and the other method (and ChildrenAsTable) are coming from the fact that the INode interface is used, which requires these methods to be defined.
.Children is meant to return a DynamicNodeList, so you can enumerate on it and call the dynamic methods, but internally, I need a statically typed (non dynamic method) to get the real children items.
The ChildrenAsTable and GetChildrenAsList are INode methods that have been there for a while. I don't want to remove them as they'll break backwards compatibility, assuming the interface is not there anymore.
I appreciate that it's frustrating when you run up against bugs, I take a great deal of care to ensure I don't break anything and all the features I release I've been testing on my own production sites.
I commit regularly to allow people to test nightlys, but there are only a handful of people testing and due to the structure of umbraco 4, it's quite hard to develop unit tests for features such as the macro engine.
Next post..
@Eran/@Dan
You're correct, it's to do with DynamicMedia and DynamicNode being the same thing
@Mikkel
Yes that should be how you call it, the DynamicMedia is only there for backwards compatibility
Finally,
Here's an updated macroengines build:
http://dl.dropbox.com/u/2923715/LatestRazorMacroEngine-Only.zip
I have fixed an issue with HasValue (not sure I've blogged about that one yet), the stack overflow with Descendants and have added the missing overloads back in.
I haven't yet updated our development site to 4.7.1, After seeing these I have concerns to update to 4.7.1.
Ideally, there should be additional methods or properties to handle things rather than changing the existing ones which can cause lots of problem for updating and maintaining a production site where there are lots of razor scripts already written with old methods.
Updating the site shouldn't break any existing code.
-Rajeev
@Rajeev: If you have a dev site, it's pretty straight forward to test out the changes. We've done our best in testing the changes made to the core and we've also had nightlies and betas out. We've announced the beta and that's a great way to help testing and finding edge-case bugs, so that once the final release is outit'll be as stable as possible. That's all we can do, really. Mistakes will happen, but the more community people that helps with testing the more stable the releases will be. If everybody leaned back and waited for someone else to try, nothing would happen.
That said, when's the last time you've seen a 12 hour turnaround from reported issue to a fixed patch reading for testing. I think Gareth has done an amazing and more-than-you-could-ask-for job.
High Five, Gareth!
@Gareth: thanks about the detailed info and the quick fix. BTW, you walkthrough posts about razor are great!
@Neils, I didn't mean that. Just a suggestion for upgrades of any product.
Sorry, I couldn't try the beta ones for razor initially as was busy on many other things.
@Gareth great work and great effort.on this. Your posts helped me to start with razor.You guys really rocks.
Thanks,
Rajeev
@Gareth - Many thanks for taking the time to post such a detailed and informative response. This is why I love the Umbraco community - you mention a problem and not only do you get an amazingly detailed response from one of the developers but you also get a working patch in less than 24 hours and updated documentation. Totally amazing!
Your logic in refactoring DynamicMedia so that it hangs off DynamicNode makes total sense. In fact, ever since I've been using Umbraco I've always thought they should be treated the same way as their are so many similarities. So well done for doing what Einstein could never manage and coming up with a Grand Unification of Umbraco :D
I was interested in this part:
I didn't even realise Media items are cached using Examine?! That is really interesting. I presume this is to work around the database hits that used to occur with traditional GetMedia() calls? I'd be interested in more info on this!
Anyway, I guess my major gripe was thinking that DynamicMedia had been majorly changed without the release notes mentioning it. But I realise this wasn't deliberate and was just an oversite, which is totally fair enough - we're all human. It's just that I love Razor so much I don't want it to go the way of previous things like LinqToUmbraco and end up unusable due to changes, lack of support etc. Sorry if I sounded like I was bitching - I had a bit of a bad day and needed to vent :)
OK, you are right, of course - I was looking at the methods of the statically typed DynamicMedia/Node (as that seemed to be the best way for me to discover what properties/methods are available). Once typed to dynamic Children worked as expected. I have to admit I don't really understand why this happens, but dynamic types are still a bit mysterious to me!
As for the similary named methods - I guess this is more of a problem because intellisense exposes methods and curious people (like me!) discover it. Perhaps in Umbraco 5 you can tidy things up without too much of a worry about breaking backward compatability.
OK, I'm off to play around with all the new features! Once again, thanks to Gareth and the rest of the team - brilliant work that once again surpasses my expectations.
Hi Michiel, the release notes for Umbraco 4.7.1 are available on CodePlex: (direct link)
Cheers, Lee.
Oh right, thanks!
Hi Gareth,
You indicate in your post from 10 months ago the following:
My question is: what is the easiest way to force a particular Media item to load directly from the database, i.e., how to bypass this behavior of "GetMedia" first looking to Examine? I've tried messing with various settings in my .config files but haven't had any success.
The reason I ask is that I am trying to track down a bug and after weeks of reading through Umbraco source code, my working theory is that the bug lies in UmbracoExamine, but without recompiling source code I don't have any way to test my theory.
Thank you!
is working on a reply...