[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: |
|
||||
| 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:
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. |