Liquid Markup on top of Textile (Feature #604)

Holger:

How is the Liquid stuff going? I have a client running my version of it in there installation and haven't heard of any bugs.

Okay, I think I now have most of the stuff ready. All tests pass, except the one checking for circular inclusion.

As I use the Liquid built-in include mechanism, we have a difference of behavior here. We used to spit out a warning on circle detection, Liquid turnes the offending include into an effective noop instead. As the test currently expects the old behavior, it breaks. What do you think, which behavior is more desirable?

The code, rebased on the current unstable branch, can be found at https://github.com/meineerde/chiliproject/tree/issues%2Funstable%2F604-liquid

Any way we can output a message on circular inclusion? This is a case where someone makes a mistake and it would be best to tell them what is wrong rather then outputting nothing.

If not, then the no-op is probably fine.

It turned out, the no-op I observed was actually caused by a bug which caused only the first include level to succeed at all. All other includes were silently dropped. This was fixed in https://github.com/meineerde/chiliproject/commit/e3c9053f6296687a8feb278a77fd3b5714f6c4aa. Subsequently, I added the previously missing check for circular inclusion and updated the test which was previously failing for the wrong reasons.

I think I'm now at 100% featurewise and at 90% on cleanliness. What is still bothering me is the way we use Drops currently. Currently, each model object to be exposed to Liquid requires the explicit implementation of a Drop class and an extension of the model.

Instead, I'd like to use https://github.com/Shopify/liquid/blob/master/lib/liquid/module_ex.rb and define which methods /attributes are to be exposed directly on the model.

Holger Just wrote:

Currently, each model object to be exposed to Liquid requires the explicit implementation of a Drop class and an extension of the model.

Instead, I'd like to use https://github.com/Shopify/liquid/blob/master/lib/liquid/module_ex.rb and define which methods /attributes are to be exposed directly on the model.

Shoot, there was a technical reason I used drops instead of liquid_methods. I started with liquid_methods but refactored/rebased them into drops.

I think it was because:

• liquid_methods created a class based off of LiquidDrop which made it hard to share behavior. My BaseDrop lets us share common methods. (e.g. custom_field should be shared by all classes with them).
• something to do with a whitelist approach: I only wanted to expose specific fields and methods to the Liquid object. (though liquid_methods seems to do this already)
• having a common BaseDrop class and actual classes will let use define object visibility. That way permissions are handled at the lowest model level: example

Also using Drops will let us expose non-Model classes too. Things like the current theme, version, etc.

Eric Davis wrote:

• liquid_methods created a class based off of LiquidDrop which made it hard to share behavior. My BaseDrop lets us share common methods. (e.g. custom_field should be shared by all classes with them).

We could either extend ::Liquid::Drop via open classes or implement our own liquid_methods. In the end, we are probably going to implement our own, with a custom base-class. However, I still like the overall design of having the real underlying class decide which methods it would like to expose.

• something to do with a whitelist approach: I only wanted to expose specific fields and methods to the Liquid object. (though liquid_methods seems to do this already)

Exactly. Only public methods of the Drop class are exposed. Those methods are created by liquid_methods. I extended that behavior in my branch so that the default behavior of "all public methods of the drop" can be further restricted. I'm not too confident in the curent implementation though.

(Note that your approach also exposes the original object to liquid, e.g. @@

• having a common BaseDrop class and actual classes will let use define object visibility. That way permissions are handled at the lowest model level: example

This is good as a last resort. From a design perspective, it shouldn't even be possible to even create a drop for an object that shouldn't be accessible for the current user.

Also using Drops will let us expose non-Model classes too. Things like the current theme, version, etc.

liquid_methods is defined on Module and is thus available for all modules and classes.

The once case where the explicit drops shines is if there is a need to define custum methods on the drop, like I've done for my prototypical QueryDrop. However, this could still be accomplished through open classes. As could be the rest. So it probably makes sense to have a hybrid approach. The generated methods could e.g. be defined on a module instead of a class. This module could then be included into a class like the one we have now. This would allow people to extend the class like usually and could overwrite certain behavior.

To be honest, this feels like a bike shed now.

Whether we use drops or the other stuff in Liquid doesn't affect what the end user sees or how they use the code. I'd say lets use the code we have now (Drops) and consider how they are implemented as an "internal API" that we can change and refactor later on. We should focus on exposing all of the current data first (so it can be used by users) and see how to allow 3rd party data to be exposed (plugins). Then we'd be able to make an informed decision on if Drops or liquid_methods is best. In fact the "best" method might even be a combination of the two or even a third unknown...

tl;dr: Review and commit the existing code, refine the internal implementation later.

Sent the pull request at https://github.com/chiliproject/chiliproject/pull/127.

It would be great if somebody could review it.