Adding in Base Processor - exposing Umbraco context services #204

jamiepollock · 2017-01-20T21:54:25Z

As discussed on issue #200. I've done the work to add UmbracoContext and ApplicationContext in the base DittoProcessorAttribute. I used the singleton UmbracoContext.Current and ApplicationContext.Current as mentioned here accessing ApplicationContext through UmbracoContext is something you'd want to avoid doing.

I've also reworked the DittoProcessor methods which called UmbracoContext.Current to use the new Umbraco property.

There are a couple of internal statics I didn't touch upon though:

Let me know what you think!

jamiepollock · 2017-01-20T22:13:39Z

I realise there's going to need be a lot more work done in order to get this to work with the tests as UmbracoContext.Current & ApplicationContext.Current are both null. :/

Based on the idea of https://github.com/umbraco/Umbraco-CMS/blob/release-7.3.2/src/Umbraco.Web/IUmbracoContextAccessor.cs & https://github.com/umbraco/Umbraco-CMS/blob/release-7.3.2/src/Umbraco.Web/SingletonUmbracoContextAccessor.cs I've implemented an "UmbracoApplicationContextAccessor which allows readonly access to the UmbracoContext & ApplicationContext. In most cases this will use the SingletonUmbracoApplicationContextAccessor but in the case of tests there is a publicly available static register method to setup an alternative which used a mocked version of this accessor. Not happy about the accessor being passed through so many properties but it's the best way I can think of to ensure we're using exactly the same contexts from method to method.

jamiepollock · 2017-01-22T16:15:35Z

Hey folks, struggling with this one. I've been back and forth with how this setup but in the end I took an idea from the core on UmbracoContext access (see notes here: bd4c79a).

Happy to rename things, the structure may not be how you guys would see the files being placed and the naming is a tad verbose. This was a bit of a brain racker!

jamiepollock · 2017-01-22T16:43:06Z

I've managed to remove the static membershiphelper but removing the static dictionary helper might be a bit more challenging. I've left it in for now and it can be reviewed later on.

leekelleher · 2017-01-22T23:05:31Z

Hey @jamiepollock, thanks for taking it on and making a start with this! 🎉

I'm not sure what to make of the IUmbracoApplicationContextAccessor yet, (it's the first time I've heard about IUmbracoContextAccessor in Umbraco core too). I'll give it some thought.

jamiepollock · 2017-01-22T23:38:05Z

@leekelleher Yea, I know what you mean. The whole reason is to have a testable in for UmbracoContext & ApplicationContext. I've been back and forth with this over the weekend after I saw the first push failed I've been trying to get this in. In the end this was the best way I could find. I noticed the Umbraco core was doing something like this in their newer code for Umbraco Identity (https://github.com/umbraco/Umbraco-CMS/search?utf8=%E2%9C%93&q=SingletonUmbracoContextAccessor) and phasing out stuff like plugincontroller. I'm not suggesting this is how we should go but I've certainly spent a good amount of time looking over a few options. I realise adding it to the constructor is out as it'd need to be public to be testable and also then appear in the attribute setup. It's been a fun one :)

…

On 22 Jan 2017, at 18:05, Lee Kelleher ***@***.***> wrote: Hey @jamiepollock, thanks for taking it on and making a start with this! 🎉 I'm not sure what to make of the IUmbracoApplicationContextAccessor yet, (it's the first time I've heard about IUmbracoContextAccessor in Umbraco core too). I'll give it some thought. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

mattbrailsford · 2017-01-23T09:55:47Z

This is one of the biggest drawbacks to the "Context" pattern imo. Testing umbraco v5, which was uber context heavy, was a real pain as you end up spending ages mocking the rabbit hole.

RE the passing in attribute, you could make an internal accessor as I think we make internals visible to the testing library so it should be allowed to be set from the testing lib.

jamiepollock · 2017-01-23T14:14:06Z

@mattbrailsford yea, I've not found it so bad. The goal with the accessor was to eliminate the need for any references to UmbracoContext in production code, as if I did put it in then it would cause tests to fail. Hence in the tests we can just setup a mocked up version and be done with it. In theory we could pass in an entirely different UmbracoContext & ApplicationContext constructed setup, somehow. not sure how that'd work mind or if it even should.

On top of this, the parameter nesting of passing the accessor result comes through comes about because we're using extension methods rather than a class we can DI something into.

I might be missing something though. I've tried going down the route of adding in internal properties this weekend and couldn't find the best place to populate those values in the production code which then wouldn't cause tests to fail. Hence coming back around to the accessor.

More than happy to take on edits, I might be close to the trees to see that wood. :)

mattbrailsford

Just a few comments

mattbrailsford · 2017-01-23T14:08:25Z

src/Our.Umbraco.Ditto/Ditto.cs

+        public static void RegisterUmbracoApplicationContextAccessor<TUmbracoApplicationContextAccessorType>()
+            where TUmbracoApplicationContextAccessorType : IUmbracoApplicationContextAccessor, new()
+        {
+            DittoProcessorRegistry.Instance.RegisterDefaultUmbracoApplicationContextAccessorType<TUmbracoApplicationContextAccessorType>();


Not sure if DittoProcessorRegistery is the right place for this?

It could be moved to Ditto then, I guess, along with the private field?

Would seem logical, unless we want a DittoUmbracoApplicationContextAccessorRegistry? @leekelleher?

Actually, given there can be only 1 type, I'm wondering if this shouldn't just be a static variable on Ditto like ProcessorAttributeTargets and DefaultCacheBy etc. The Register methods are only really relevant if you have multiple types to register.

doh! looks like we do have RegisterDefaultProcessorType though, hmmm. Lets keep it how you've got it, but just move it to Ditto including the private property. :)

Okay. I'll get on that a bit later. :)

Thanks for the feedback, @mattbrailsford!

mattbrailsford · 2017-01-23T14:11:55Z

src/Our.Umbraco.Ditto/ComponentModel/Attributes/DittoProcessorAttribute.cs

+        /// </summary>
+        protected MembershipHelper Members
+        {
+            get { return new MembershipHelper(Umbraco); }


I'm wondering if these should be stored on the processor context instead, and then these could maybe be convenience accessors?

Sure, so it'd be Context.Members, etc?

You could keep these accessors, but maybe the main exposure point is the ProcessorContext, so you could do Context.Members or just Members (which internally just calls Context.Members). I dunno if this is a good or bad idea, so open to discussion? @leekelleher?

mattbrailsford · 2017-01-23T14:13:05Z

src/Our.Umbraco.Ditto/ComponentModel/Processors/DittoProcessorRegistry.cs

@@ -31,6 +31,11 @@ internal class DittoProcessorRegistry
        private Type defaultProcessorType = typeof(UmbracoPropertyAttribute);

        /// <summary>
+        /// The default umbraco application context accessor type for processors, (defaults to `SingletonUmbracoApplicationContextAccessor`).
+        /// </summary>
+        private Type defaultUmbracoApplicationContextAccessorType = typeof(SingletonUmbracoApplicationContextAccessor);


Thinking this should maybe go directly on Ditto?

Yea, I was wondering this too. Happy for it to be moved. :)

Conflicts: src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs

jamiepollock · 2017-01-23T23:07:44Z

@mattbrailsford I've looked at implementing the code review changes. Only one I've not implemented is helpers into the Context. I wasn't quite sure how to achieve that at this point or if we're going that way.

mattbrailsford · 2017-01-24T08:46:35Z

@jamiepollock no worries. I'm not 100% on that suggestion so happy for it to stay as is and we can always move it later if needs be. It wouldn't change the API on the processors so won't be a big deal to move if we need to.

mattbrailsford · 2017-01-24T16:19:00Z

Sorry, one last thing. I think I'd drop the use of the word "Default" for the UmbracoApplicationAccessor stiff as there can be only one anyway, so it's not like it's a default of a collection (like the Processor one is). Do you think?

jamiepollock · 2017-01-24T16:34:31Z

Sounds like a good idea. It was based, as you can imagine, on the default processor type but we can't specify an accessor on a per model basis. I'll add it in later on when I get back home.

…

On 24 Jan 2017, at 11:19, Matt Brailsford ***@***.***> wrote: Sorry, one last thing. I think I'd drop the use of the word "Default" for the UmbracoApplicationAccessor stiff as there can be only one anyway, so it's not like it's a default of a collection (like the Processor one is). Do you think? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

jamiepollock · 2017-01-24T18:12:49Z

@mattbrailsford sorted!

Removed the static class, UmbracoDictionaryHelper. Tentatively added in UmbracoHelper instance as in Umbraco v7.3.2 this is the best way to get a dictionary value. Also worked in some teardowns for Resolution.Reset() so tests work between this and the UmbracoPicker tests. Finally, there's a slight change to the dictionary tests as UmbracoHelper.GetDictionaryValue doesn't do an ordinal key check for keys. Not sure how we can fix this within Ditto, it might be a feature Umbraco adds in later on.

jamiepollock · 2017-01-25T02:47:00Z

I've added an instance of UmbracoHelper to UmbracoDictionaryAttribute.cs.

I feel this might be something we could add at Processor level as it has useful methods outside of wrapping up Services methods or ContentCache.

Conflicts: src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs tests/Our.Umbraco.Ditto.Tests/Our.Umbraco.Ditto.Tests.csproj

jamiepollock · 2017-01-25T22:49:19Z

@mattbrailsford @leekelleher Okay, chaps. I bit the bullet and added in a reference to UmbracoHelper along side all the other new additions. Much like Members (MembershipHelper) & UmbracoHelper referenced elsewhere this is simply called "Umbraco".

This means UmbracoContext has been renamed from Umbraco to UmbracoContext.

All references have been updated! I think we're getting closer! :D

Added an XPath processor that can query the Umbraco content cache. Included a supporting unit-test. We do need to review how the mocking objects/services are created, I believe PR #204 may cover this?

leekelleher

All looking good.
(Apologies for not having the time to review earlier)

I've left a couple of comments about verbose naming... I'm not 100% sure how they should be named, we can discuss.

One thing I'd like to do is remove the Convert___FromInt methods from DittoProcessorAttribute - I don't think we need them any more. A processor can return an IPublishedContent, then either let Ditto (or another processor) handle it accordingly. (There's no need for those methods to explicitly call the .As() method.

leekelleher · 2017-02-10T01:06:34Z

src/Our.Umbraco.Ditto/Ditto.cs

+        /// Registers a global umbraco application context accessor.
+        /// </summary>
+        /// <typeparam name="TUmbracoApplicationContextAccessorType">The type of the accessor.</typeparam>
+        public static void RegisterUmbracoApplicationContextAccessor<TUmbracoApplicationContextAccessorType>()


This feels like a mouthful, wondering if this can have a concise name?

Hey @leekelleher, I'd like to have a more concise name however it's an class that acts an accessor for both UmbracoContext and ApplicationContext. I'm at a loss as to what else to call it but I'm happy to hear any suggestions :)

@leekelleher UmbracoPickerAttribute is the last reference to those Convert___FromInt methods. Definitely happy to drop them as UmbracoPickerAttribute does everything

@JimBobSquarePants I currently set them as deprecated, (142768e), but think I may remove them instead now.
@mattbrailsford Any objections?

Chop chop chop...em

...and they're gone! 9678d43

leekelleher · 2017-02-10T01:09:44Z

src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs

@@ -11,6 +11,7 @@

 namespace Our.Umbraco.Ditto
 {
+    using global::Umbraco.Core;


We can place the using namespaces outside the main namespace. Those left inside are older code. The global:: part felt unnecessary.

leekelleher · 2017-02-10T01:11:30Z

src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs

@@ -191,12 +192,15 @@ public static class PublishedContentExtensions
                throw new ArgumentException(string.Format("The instance parameter does not implement Type '{0}'", type.Name), "instance");
            }

+            // Get the accessor for UmbracoContext & ApplicationContext
+            var umbracoApplicationContextAccessor = (IUmbracoApplicationContextAccessor)Activator.CreateInstance(Ditto.GetUmbracoApplicationContextAccessorType());


I think we have an extension method that can create new object instances - I'm lead to believe it has performance optimisations. I'll find out what the method is.

The umbracoApplicationContextAccessor variable name feels too verbose... let's consider alternatives 😏

GetInstance(this Type type) though internally that calls Activator.CreateInstance() if no parameters are passed. We should use it for consistency though.

@JimBobSquarePants Cool, done - in comment fd44882 👍

leekelleher · 2017-02-10T01:15:41Z

src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs

                }
                else
                {
-                    return ConvertContent(content, type, culture, instance, processorContexts, onConverting, onConverted);
+                    return ConvertContent(content, type, umbracoApplicationContextAccessor, culture, instance, processorContexts, onConverting, onConverted);


Is there a reason why the umbracoApplicationContextAccessor is being passed in as a method parameter from this high up the chain? I'm assuming it's to do with the lazy-loaded properties, but let's review 👍

@leekelleher I wanted to find the highest up point in the chain where we used UmbracoContext.Current (where the CurrentCulture is being used) then use then pass the same context through to the extension methods to the place where we used to inject it into the processor attributes.

It didn't seem right to new the class in both cases but I see how passing this through might seem a little odd too. :)

@jamiepollock I think it's fine... just wanted to flag it (probably me being pedantic late last night 😉)

@mattbrailsford You cool with this?

leekelleher · 2017-02-10T01:17:47Z

tests/Our.Umbraco.Ditto.Tests/DictionaryValueTests.cs

+        {
+            if (Resolution.IsFrozen)
+            {
+                Resolution.Reset();


On a complete tangent, I had a problem with the frozen resolution on PR #209 ... and seeing this Resolution.Reset() might fix my issue... thanking you! 💯

jamiepollock · 2017-02-10T04:30:50Z

I think the main stickler here is the naming of the UmbracoApplicationContextAccessor, As mentioned in this commit (bd4c79a) I took a cue from the core but that's probably not commonly used code as it was mostly put in for Umbraco Identity.

I don't think UmbracoContextAccessor is quite right as it also accesses ApplicationContext. In the current version of Umbraco we're using IUmbracoContextAccessor is internal but in newer versions it's public.

Happy to rename this to whatever you guys think is best :)

leekelleher · 2017-02-10T10:46:07Z

I've created a new feature branch called "feature/umbraco-context", I'll merge this in to there... then we can review/amend accordingly (then get it merged into "develop" branch).

@jamiepollock Thanks again for this, it's very much appreciated! #h5yr 💯

…onsistency. ref: #204 (comment)

jamiepollock · 2017-03-18T13:17:58Z

Hey guys, is there anything I can do to help this along? Just wondering where we're at. I'm sure you're all super busy at the moment. :)

leekelleher · 2017-03-18T13:20:04Z

@jamiepollock It's the first time I've had chance to look at Ditto in ages. Reviewing stuff now and we'll see where it's at. 🤘

jamiepollock · 2017-03-18T13:23:42Z

@leekelleher thats okay :) just want to make sure it's in the pipeline. I was happy with the code where I left off but time makes way for uncertainty.

jamiepollock added 4 commits January 20, 2017 16:34

Added in exposable Umbraco properties

28581a1

Constructor logic to wire in UmbracoContext & ApplicationContext

9b2b4bb

Reworked UmbracoContext.Current calls to use new UmbracoContext property

6a8ff0e

Further update to use new exposed properties

e25d132

jamiepollock added 2 commits January 22, 2017 11:08

Added mock UmbracoApplicationContextAccessor and global setup fixture

4adf7a7

Remove static membershiphelper as it's not being used any more

e3093b5

leekelleher mentioned this pull request Jan 23, 2017

Base Processor - exposing Umbraco context services #200

Closed

mattbrailsford reviewed Jan 23, 2017

View reviewed changes

jamiepollock added 3 commits January 23, 2017 17:52

Merge remote-tracking branch 'refs/remotes/upstream/develop'

984b940

Conflicts: src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs

Corrected previous mis-merge

e83e19d

Moved the defaultUmbracoApplicationContextAccessorType into Ditto.cs

9558b02

Removed "default" from any namings for UmbracoApplicationContextAccessor

1994996

jamiepollock added 2 commits January 25, 2017 17:37

Merge remote-tracking branch 'refs/remotes/upstream/develop'

10736b1

Conflicts: src/Our.Umbraco.Ditto/Extensions/PublishedContentExtensions.cs tests/Our.Umbraco.Ditto.Tests/Our.Umbraco.Ditto.Tests.csproj

Rename Umbraco to UmbracoContext, add in UmbracoHelper & fix last commit

0163d2c

jamiepollock mentioned this pull request Feb 2, 2017

Custom Processor Contexts only persist a single .As() #201

Closed

leekelleher mentioned this pull request Feb 10, 2017

XPath Processor #209

Merged

leekelleher self-requested a review February 10, 2017 01:01

leekelleher reviewed Feb 10, 2017

View reviewed changes

leekelleher changed the base branch from develop to feature/umbraco-context February 10, 2017 10:43

leekelleher merged commit 0a13fcf into umco:feature/umbraco-context Feb 10, 2017

leekelleher added a commit that referenced this pull request Feb 10, 2017

Switched to use GetInstance(this Type type) extension method, for c…

fd44882

…onsistency. ref: #204 (comment)

leekelleher mentioned this pull request Mar 18, 2017

Merging in "umbraco-context" branch #212

Merged

leekelleher mentioned this pull request Mar 18, 2017

IUmbracoApplicationContextAccessor rename #213

Merged

leekelleher added this to the 0.11.0 milestone May 30, 2017

Adding in Base Processor - exposing Umbraco context services #204

Adding in Base Processor - exposing Umbraco context services #204

Conversation

jamiepollock commented Jan 20, 2017

jamiepollock commented Jan 20, 2017

jamiepollock commented Jan 22, 2017

jamiepollock commented Jan 22, 2017 • edited Loading

leekelleher commented Jan 22, 2017

jamiepollock commented Jan 22, 2017 via email

mattbrailsford commented Jan 23, 2017

jamiepollock commented Jan 23, 2017

mattbrailsford left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiepollock commented Jan 23, 2017

mattbrailsford commented Jan 24, 2017

mattbrailsford commented Jan 24, 2017

jamiepollock commented Jan 24, 2017 via email

jamiepollock commented Jan 24, 2017

jamiepollock commented Jan 25, 2017

jamiepollock commented Jan 25, 2017

leekelleher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JimBobSquarePants Feb 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamiepollock commented Feb 10, 2017

leekelleher commented Feb 10, 2017

jamiepollock commented Mar 18, 2017

leekelleher commented Mar 18, 2017

jamiepollock commented Mar 18, 2017

jamiepollock commented Jan 22, 2017 •

edited

Loading

JimBobSquarePants Feb 10, 2017 •

edited

Loading