mindrevolution / SirTrevor-for-Umbraco

A SirTrevor editor plugin for Umbraco 7+ that integrates.
21 stars 6 forks source link

Razor file blocks for the SirTrevor namespace #21

Open abjerner opened 10 years ago

abjerner commented 10 years ago

The file SirTrevor.cshtml in the App_Code folder will result in an auto-generated class having the full name of ASP.SirTrevor (since all Razor files are created in the ASP namespace).

So for our own Razor files, having a reference to a class called SirTrevor will mean that we can't reference classes in the SirTrevor namespace (as we're using in the DLL). At least not like this:

@typeof(SirTrevor.DataValue.SirTrevorDataValue)

However something the code below will work, but prefixing all references with global:: is not very user-friendly:

@typeof(global::SirTrevor.DataValue.SirTrevorDataValue)

I'm not totally sure on how to proceed with this, so here goes my thoughts:

  1. One option could be to find another default namespace for the DLL, so for instance the class SirTrevor.DataValue.SirTrevorDataValue would be moved to Something.SirTrevor.DataValue.SirTrevorDataValue and so forth. Since my changes with the DLL hasn't been released yet, this shouldn't break any code in existing installs.
  2. Another option would be to remove the SirTrevor.cshtml file from the App_Code folder. The two helper functions can easily be moved to a helper class in the DLL, but I'm pretty sure that this will cause a breaking change for people who already have installed the package and used the example MVC view.
  3. Something I haven't thought of...

Option 1 will have the least impact on existing installs being upgraded, but option 2 will give a clean package without any unnecessary files that could/should be in the DLL.

Hope this makes sense ;)

MarcStoecker commented 10 years ago

Makes total sense to discuss this. Thanks for bringing this up! :)

If we're going the route to be more feature complete and dev friendly (outside the frontenders et al.) it's probably an idea to go with an integrated assembly, wrapping everything up into one package.

Could save us from update nightmares in the future as well (old files, and so on).

Can't we provide a wrapper App_Code-Helper-File for the time being that uses the new DLL?

abjerner commented 10 years ago

Changing the Razor file so that calls to the helper functions are passed on to matching methods in the DLL isn't a problem. I took me only a few minutes to rewrite, so another commit coming up soon ;)

Keeping the filename SirTrevor.cshtml of the Razor file (which is a good idea for backwards compatibility) will still block developers from referring to the classes in the DLL in their own Razor files, so I think we should change the default namespace. Any preferences?

MarcStoecker commented 10 years ago

As far as I know there is unfortunatly no way to have a custom extension @SirTrevor. instead of @Html.SirTrevor(). (without a custom WebViewPage, sure) which is not only breaking backwards compatibility but also quite ugly. Nasty method-as-namespace hack or everything else clutters the @Html. extension. Both awful, IMHO.

I tried to find our how UmbracoHelper does it, but 3 hours later I still haven't figured out substantially. Do you have any idea? Would be the cleanest and most compatible solution to have @SirTrevor. in the assembly ...

If we can't figure out (maybe Shannon, Jeavon or the others can point uns into the right direction?) how to do that - what would be your proposed namespace naming? Everything I come up with feels akward, honestly... ;)

MarcStoecker commented 10 years ago

Heretical: Is a breaking change an option?

abjerner commented 10 years ago

The package is still in beta, so breaking changes may be expected. But we can of course try to keep it to a minimum or even avoid it at all.

From what I can tell @Umbraco. comes from this property: https://github.com/umbraco/Umbraco-CMS/blob/d1c5ddc03dfb79195e1c97eededcabc9dcaa1365/src/Umbraco.Web/Mvc/UmbracoViewPageOfTModel.cs#L96 I can't think of anyway to duplicate that without a custom WebViewPage as you point out.

I played around with a few ways to accomplish what we want. Currently I see the following options:

  1. If we go back to changing the namespace, we can leave the SirTrevor.cshtml where it is now, and then just route the helper functions to the matching methods in the DLL. Something like this:
@helper Markdown(string markdown) {
    @(global::SirTrevor.SirTrevorHelpers.Markdown(markdown))
}

@helper RemoveMarkdown(string markdown) {
    @(global::SirTrevor.SirTrevorHelpers.RemoveMarkdown(markdown))
}

Since the classes in the DLL hasn't been released yet, this will not create any breaking changes.

Regarding what namespace to use, I know that Warren Buckley has used the Umbraco.Community.PackageName namespace in the past. I also think there has been some packages in the past using the Our.PackageName namespace, but can't find any examples on that with a quick search. Since you started out with this package, the namespace could also be Mindrevolution.SirTrevor - I wouldn't have a problem that ;)

  1. If we hack around a little, declaring a SirTrevor class in the Umbraco.Web.Mvc will make it visible in our Razor files. Perhaps not that pretty, but it seems to do the job. This will however create a breaking change - or at least require the user to delete the SirTrevor.cshtml file (I haven't tested whether Umbraco will remove the file automatically if it is removed from the package).
  2. Another option is to create a class with the fullname SirTrevor.SirTrevor. Users will then be able to call @SirTrevor.Markdown("**Umbraco**") if they also call @using SirTrevor. Again for this to work, an existing user must make sure that the SirTrevor.cshtml is deleted.

It's getting late here, so I hope I haven't rambled too much. To play well with existing installations I think the first option is the best, and it will require very little work.

MarcStoecker commented 10 years ago

Deleting a file could be done with the post-install old-school ascx; "if SirTrevor.cshtml exists, delete it". We would just need to deliver an empty SirTrevor.cshtml to stamp out the class prior to the app pool restart and get rid of it afterwards. No experience with such an action, though.

"Should work(tm)"

MarcStoecker commented 10 years ago

Yes, Lee Kelleher has used the Our namespace in the past, AFAIR. I'd like to got with just SirTrevor for simplicity and having AssemblyName and RootNamespace the same if we can make it happen. A clean SirTrevor. namespace would be handy for everybody. Beginning with the next major release (i.e. an early 1.0-beta) we will most likely only ship a single assembly plus the blocks folder, right?

I think it's smart to add the helper part to the `Umbraco.Web.Mvc``namespace. Haven't thought about that... Did you try and test? :)

abjerner commented 10 years ago

My experience with package actions is very limited, so I don't know whether something like that is possible. Another way to handle it may simply be to let users know in the upgrade guidelines that they should uninstall the existing package first, then install the new one.

I think we still need the entire App_Plugins/SirTrevor folder, but I haven't checked in to how much is possible from C#.

I have tested and confirmed that creating a class in the Umbraco.Web.Mvc will make it visible in MVC views. I haven't tried the same in partial views or macros, but will do that now.

MarcStoecker commented 10 years ago

If people need to uninstall, the datatype gets an new GUID, right? So they will need to reassign the datatype everywhere - or did that behaviour change? :)

Yes, we'll still need to keep the /App_Plugins/SirTrevor/ folder, but all the .css, .js, ... will be embedded resources coming from the DLL. More or less only the blocks will stay in blocks/.

abjerner commented 10 years ago

You may be right about the datatype and a new GUID. I have had some bad experiences with datatypes in the past, so I have avoided creating datatypes on install in my own packages (mostly internal packages). But I haven't tested whether it is something that Umbraco has fixed.

abjerner commented 10 years ago

I did some further testing/research. Which namespaces that are available in Razor files is determined by two Web.config files:

  1. ~/Views/Web.config which applies to views, partial views and macro partial views (if located in the Views folder)
  2. ~/macroScripts/Web.config which applies to the "old" Razor files used in macros

This is an example of the Razor-related section in ~/Views/Web.config:

    <host factoryType="System.Web.Mvc.MvcWebRazorHostFactory, System.Web.Mvc, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31BF3856AD364E35" />
    <pages pageBaseType="System.Web.Mvc.WebViewPage">
      <namespaces>
        <add namespace="System.Web.Mvc" />
        <add namespace="System.Web.Mvc.Ajax" />
        <add namespace="System.Web.Mvc.Html" />
        <add namespace="System.Web.Routing" />
        <add namespace="Umbraco.Web" />
        <add namespace="Umbraco.Core" />
        <add namespace="Umbraco.Core.Models" />
        <add namespace="Umbraco.Web.Mvc" />
        <add namespace="Microsoft.Web.Helpers" />
        <add namespace="umbraco" />
        <add namespace="Examine" />
      </namespaces>
    </pages>
  </system.web.webPages.razor>

So we can add our own namespaces here, and classes in that namespace will automatically be available in Razor files located in this directory (as well as sub directories).

    <host factoryType="System.Web.Mvc.MvcWebRazorHostFactory, System.Web.Mvc, Version=4.0.0.0, Culture=neutral, PublicKeyToken=31BF3856AD364E35" />
    <pages pageBaseType="System.Web.Mvc.WebViewPage">
      <namespaces>
        <add namespace="System.Web.Mvc" />
        <add namespace="System.Web.Mvc.Ajax" />
        <add namespace="System.Web.Mvc.Html" />
        <add namespace="System.Web.Routing" />
        <add namespace="Umbraco.Web" />
        <add namespace="Umbraco.Core" />
        <add namespace="Umbraco.Core.Models" />
        <add namespace="Umbraco.Web.Mvc" />
        <add namespace="Microsoft.Web.Helpers" />
        <add namespace="umbraco" />
        <add namespace="Examine" />
        <add namespace="SirTrevor.Razor" />
      </namespaces>
    </pages>
  </system.web.webPages.razor>

I have a created a package action that will add this namespace on installation, and of course remove it again if the user chooses to uninstall the package. The package action also removes the SirTrevor.cshtml file.

I have tested this on a local 7.1.4 where it works like a charm. It doesn't rely on any new Umbraco logic, so it should work on all 7.x installations.

Once we have sorted out the branch issue, I can commit this to the 0.8 branch.

I'm not totally sure at the moment how to handle the same if we introduce a NuGet package. Hopefully NuGet can help us do an XML transformation on both install and uninstall. It is important to remove the namespace again since it will cause an exception in Razor files if the namespace can't be found.