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

Introduce FluidValue for Hosting Environment #16972

Merged
merged 5 commits into from
Nov 8, 2024
Merged

Conversation

hishamco
Copy link
Member

@hishamco hishamco commented Nov 8, 2024

No description provided.


namespace OrchardCore.DisplayManagement.Liquid.Values;

internal sealed class HostingEnvironmentValue : FluidValue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the advantage of this implementation over the current one?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is simpler to understand with one less indirection. Instead of registering a marker object (the accessor) and checking any property used in Liquid for this type, we register an object directly and let it return sub-properties when it is accessed.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, thanks. @hishamco please document if anything changes here: https://docs.orchardcore.net/en/latest/reference/modules/Liquid/#environment

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing to be changed in the usage, no breaking-changes


namespace OrchardCore.DisplayManagement.Liquid.Values;

internal sealed class HostingEnvironmentValue : FluidValue
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is simpler to understand with one less indirection. Instead of registering a marker object (the accessor) and checking any property used in Liquid for this type, we register an object directly and let it return sub-properties when it is accessed.


return NilValue.Instance;
});
o.Scope.SetValue("Environment", new HostingEnvironmentValue());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't the value of the environment be found at that point, the same way we would use it to register specific middlewares or not. Then we can just pass it once to the FluidValue and preset the IsXZY values in it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I go with the current implement because I can't resolve IHostEnvironment, let me check again. Agree that resolving the value once is enough in case of the environment not like the culture

…stingEnvironmentValue.cs

Co-authored-by: Sébastien Ros <[email protected]>
@hishamco hishamco enabled auto-merge (squash) November 8, 2024 22:47
@hishamco hishamco merged commit c1aa6ad into main Nov 8, 2024
12 checks passed
@hishamco hishamco deleted the hishamco/env-liquid branch November 8, 2024 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants