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

Inside UmbracoApiController, trying to output value of RTE value containing a macro causes error #7362

Closed
Zweben opened this issue Dec 18, 2019 · 6 comments
Labels
community/up-for-grabs status/stale Marked as stale due to inactivity

Comments

@Zweben
Copy link
Contributor

Zweben commented Dec 18, 2019

On code running inside an UmbracoApiController or UmbracoAuthorizedApiController, any attempt to output the value of a "Rich Text Editor" property that contains an embedded macro causes the error:

System.ArgumentNullException: 'Value cannot be null.
Parameter name: doc'

Umbraco version

I am seeing this issue on Umbraco version 8.4.0.

Steps to reproduce

  1. Add a language-invariant Rich Text Editor property to a language-variant doctype (not sure if the language variance matters).
  2. Embed any macro in the RTE.
  3. Create an UmbracoApiController or UmbracoAuthorizedApiController
  4. Using either the .Value method or the Models Builder syntax, try to output the RTE value.

Sample code

    public class TestController : UmbracoApiController
    {
        [HttpGet]
        public string Test()
        {
            // ID of node with RTE property containing embedded macro
            int id = 1234; 

            IPublishedContent node = Umbraco.Content(id);

            return node.Value<string>("bodyText");

            // This has the same result:
            // return ((StandardPage)node).BodyText.ToString();
        }
    }
@Zweben
Copy link
Contributor Author

Zweben commented Dec 18, 2019

I just found issue #6141, which seems to be heavily related. I tried getting the content via GetByRoute, however, and I am getting the exact same result. It works when I try to access an RTE value without a macro, but gives an error when the RTE contains a macro.

@nul800sebastiaan
Copy link
Member

Just to let you know, macros need all kinds of contexts to be available and it's not going to be easy to make this work. I think v7 we just logged an error saying we can't render macros without an HTTP context and a pointer to what the content is you're rendering.

Here's a hint I got from my colleague to see if you can get it to work:

Normally this a value converter job but macros are strange and require a bunch of strange stuff added to the httpcontext and in this case that isn't setup for this scenario because when in the context of just an api controller there is no 'current' page, it's just a random request.

The only way to do this would be to trick the macro renderer and give the current request a context that has a current page. I can't remember how to do this off the top of my head, will have to look.

People often get confused that Http requests are stateless. Just because you create an ajax request from a page that was rendered by a published route (meaning there was a current page) doesn't mean that ajax request has any notion of what page was rendered that made this stateless call.

There is the EnsurePublishedContentRequest attribute. https://github.com/umbraco/Umbraco-CMS/blob/v8/dev/src/Umbraco.Web/Mvc/EnsurePublishedContentRequestAttribute.cs

However, this is only for MVC controllers not WebApi controllers. But that is the kind of code you'd have to jump through in order to make this work. Or you can reference the code within UmbracoVirtualNodeRouteHandler. Essentially you would have to create a published route request, assign a resolved IPublishedContent item to it, then assign this published request to the umbraco context and then you'd have to PrepareRequest all in order to give the current request a context of a current page. Then macros will work, otherwise they will not.

We should be doing a null check where they are getting the error though so it's not just crashing and maybe adding a warning to the log that a macro cannot be rendered without a current page context.

So 2 things to conclude here:

  1. To get macros to render in an ApiController you might be successful if you jump through the hoops above
  2. We should not throw a null reference error when we can't render the macro, but catch the error and log a warning

For #2 I'll be marking this issue to be worked on by the community in case you or someone else can help.

@stevemegson
Copy link
Contributor

On point 2, would it be better to remove all the null checks for IPublishedContent in MacroRenderer and let the macro try to execute with Model.Content left null? It's quite possible to write useful macros that don't need the current page at all, such as taking some input from parameters and querying some external data source.

If the macro does need a current page then the failure will be handled by the chosen MacroErrorBehaviour, throwing or silently discarding the exception as appropriate. Once we know the macro has failed to render, we could then also log a warning to help explain what's gone wrong.

@Zweben
Copy link
Contributor Author

Zweben commented Dec 20, 2019

@stevemegson That suggestion sounds good to me. I would have no issue with writing my Marcos so that they can handle not having the usual context when being accessed from an API Controller. It’s much better than not being able to use them at all.

@umbrabot
Copy link

Hi @Zweben,

We're writing to let you know that we've added the Up For Grabs label to your issue. We feel that this issue is ideal to flag for a community member to work on it. Once flagged here, folk looking for issues to work on will know to look at yours. Of course, please feel free work on this yourself ;-). If there are any changes to this status, we'll be sure to let you know.

For more information about issues and states, have a look at this blog post

Thanks muchly, from your friendly PR team bot :-)

@umbrabot
Copy link

umbrabot commented Mar 7, 2021

Hiya @Zweben,

Just wanted to let you know that we noticed that this issue got a bit stale and might not be relevant any more.

We will close this issue for now but we're happy to open it up again if you think it's still relevant (for example: it's a feature request that's not yet implemented, or it's a bug that's not yet been fixed).

To open it this issue up again, you can write @umbrabot still relevant in a new comment as the first line. It would be super helpful for us if on the next line you could let us know why you think it's still relevant.

For example:

@umbrabot still relevant
This bug can still be reproduced in version x.y.z

This will reopen the issue in the next few hours.

Thanks, from your friendly Umbraco GitHub bot 🤖 🙂

@umbrabot umbrabot added the status/stale Marked as stale due to inactivity label Mar 7, 2021
@umbrabot umbrabot closed this as completed Mar 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
community/up-for-grabs status/stale Marked as stale due to inactivity
Projects
None yet
Development

No branches or pull requests

4 participants