[MAGNOLIA-3703] When implementing EarlyExecutionAware, execute() is never called Created: 19/May/11  Updated: 09/Feb/17  Resolved: 04/Nov/15

Status: Closed
Project: Magnolia
Component/s: templating
Affects Version/s: None
Fix Version/s: 5.5

Type: Bug Priority: Neutral
Reporter: Magnolia International Assignee: Unassigned
Resolution: Won't Do Votes: 0
Labels: api-change, support
Remaining Estimate: Not Specified
Time Spent: Not Specified
Original Estimate: Not Specified

Issue Links:
causality
Template:
Acceptance criteria:
Empty
Task DoD:
[ ]* Doc/release notes changes? Comment present?
[ ]* Downstream builds green?
[ ]* Solution information and context easily available?
[ ]* Tests
[ ]* FixVersion filled and not yet released
[ ]  Architecture Decision Record (ADR)
Bug DoR:
[ ]* Steps to reproduce, expected, and actual results filled
[ ]* Affected version filled
Date of First Response:

 Description   

If a model class does not implement EarlyExecutionAware, its execute() method is called, before rendering, if a mgnlModelExecutionUUID parameter is passed. It is, correctly, not executed a second at the time of rendering.

If a model class implements EarlyExecutionAware, its executeEarly() method is called, appropriately, before rendering happens. Its setParent() method is also called, just before rendering, to "re-integrate" the early executed model with its now-executed parent. However, since this class also implements the regular RenderingModel model interface, I would expect its execute() to be called at rendering time. This is especially critical when a model extends a "regular" model class, but additionally implements EarlyExecutionAware – The subclass should not have to remember to call its superclass's execute() method in its executeEarly() method - and it would have to "cancel" its own execute() method.

I'd rather have "business logic of a paragraph" in my executeEarly() method, and stuff to "prepare" rendering (fetching content, ...) in execute().

I see one possible problem with this approach though: executeEarly() "Can return a string which is passed #determineTemplatePath" - what do we do in case both executeEarly() and execute() returned a non-null and different value ?



 Comments   
Comment by Philipp Bärfuss [ 20/May/11 ]

This was indeed not the intention and this is why we introduced the interface and extra method. The determineTemplatePath() method should get the result of the execute() method passed.

Comment by Magnolia International [ 20/May/11 ]

Then what do we do with the return value of executeEarly() ?

Perhaps execute() should be able to get that value. Of course, one can store it in an instance field.. but in that case, executeEarly() should probably be void, to avoid confusion.

Comment by Magnolia International [ 20/May/11 ]

Ha, and another thing: what do we do with the actionResult, too; which do we expose to templates ? I guess, for consistency, you'd expose the one from execute().

For both this question and the previous, it's fairly critical to get this right for models that extend an existing model and additionally implement EarlyExecutionAware (i.e when their superclass doesn't)

Comment by Philipp Bärfuss [ 23/May/11 ]

Yes, this is how I see it. It is the execute method which influences the rendering (select the script and being available as a value in the context) in my opinion executeEarly() should return void.

Comment by Philipp Bärfuss [ 23/May/11 ]

But first we fix the missing execution of the execute method and in 5.0 we make the API change.

Comment by Jan Haderka [ 09/Aug/15 ]

Just to summarise discussion above:

  • we change signature of executeEarly() to return void
  • determineTemplatePath() will be passed in only result of execute() method to avoid confusion
  • if implementing code needs to pass something from executeEarly() to execute() or to determineTemplatePath() via execute() return value, it will be up to implementor to store this value in the instance of model to be passed on later

Implementing this ticket will result in API breaking change that will not be backward compatible and needs to be explicitly mentioned in the release notes!

Comment by Michael Mühlebach [ 04/Nov/15 ]

Given the thousands of other issues we have open that are more highly requested, we won't be able to address this issue in the foreseeable future. Instead we will focus on issues with a higher impact, and more votes.
Thanks for taking the time to raise this issue. As you are no doubt aware this issue has been on our backlog for some time now with very little movement.
I'm going to close this to set expectations so the issue doesn't stay open for years with few updates. If the issue is still relevant please feel free to reopen it or create a new issue.

Generated at Mon Feb 12 03:48:53 CET 2024 using Jira 9.4.2#940002-sha1:46d1a51de284217efdcb32434eab47a99af2938b.