Fixed Template modifications application execution order changes

[Snog]

I'm not sure what's going on, but since upgrading to 2.1.8 Patch 1 it seems template modifications made by multiple add-ons to the same location of a template are no longer being applied. And this is throwing errors in other related add-on templates.

IE: This modification to the thread_list_macros template:
FIND:

#<xf:prefixinput .*placeholder="{\$forum.thread_prompt.*\/>#Us

REPLACE:

<xf:if is="(in_array($forum.node_id, $xf.options.addon_forum))">
    <xf:include template="snog_addon_quick_reply" />
<xf:else />
    $0
</xf:if>

Multiple add-ons use that same code but with different template includes.

If I disable all but one of the add-ons, the template modification works properly. If I enable even one other add-on that uses the same code, errors are thrown.

Sorry, that's be best description I have at the moment. I'll see what I can find to better define the problem.

 

[Chris D]

Not seeing an issue with that exact TM here so we might need more details including information about the errors it is throwing.



If you can provide a screenshot of the TM log too that may give details of what/why it isn't applying:

 

[Mike]

I've added that template modification 3 times locally. The only difference I did was add a [1] (2 or 3) to the beginning of the replace to make it clear it was being applied multiple times. All 3 cases applied simultaneously without issue.

 

[Snog]

I am guessing the new way template modifications are applied is what is causing the problem. The problem seems to be cascading to different template modifications. I fix one, and another one starts causing problems.

For now, I would say go ahead and mark this as not a bug. If I find something concrete I'll repost it.

However, these are add-ons and template modifications that have functioned without a problem for years until 2.1.8 was released.

And indeed reverting the applyModificationsToTemplate routine (removing addon_id from the order clause), and rebuilding the add-ons so modifications are re-applied, all problems disappear.

 

[Chris D]

I am guessing the new way template modifications are applied is what is causing the problem.

Are you referring to us changing this:

->order('execution_order')

To this:

->order(['addon_id', 'execution_order'])

In the applyModificationsToTemplate method?

That just simply would not cause any of the issues you've described.

I should note that you've really not explained much about the problems you're seeing though.

I did ask for a bit more information in my initial reply.

I will move this to dev discussions for now as I'm currently of the opinion that there isn't a bug here but it deserves some further debugging.

 

[Snog]

Are you referring to us changing this:

->order('execution_order')

To this:

->order(['addon_id', 'execution_order'])

In the applyModificationsToTemplate method?

That just simply would not cause any of the issues you've described.

I should note that you've really not explained much about the problems you're seeing though.

I did ask for a bit more information in my initial reply.

I will move this to dev discussions for now as I'm currently of the opinion that there isn't a bug here but it deserves some further debugging.

Yes, simply changing this:

->order(['addon_id', 'execution_order'])

To this:

->order(['execution_order'])

And everything functions as it always has.

So far as additional information, it's really quite hard to explain. It's almost as if add-ons are cross changing templates. I'm going to work on it a bit more tomorrow to see if I can track down the exact reason the old order works, but the new one doesn't.

I have looked at the compiled template code with the new order, and I haven't seen any reason why they shouldn't work without any problems.

 

[Ozzy47]

@Xon might be interested in this, I think he had one or more of his addons have errors due to this.

 

[Chris D]

Do you have any situations where your template modifications in one add-on rely on modifications made in other add-ons?

That's feasible but it should be rare.

 

[Mike]

We likely will be adjusting the execution order back roughly to how it was, so I'll move this back to bugs for now.

Though it's worth trying to figure out what's order dependent in your TM as in most cases that does indicate fragility. Conversely, there may be some cases where a very particular order is needed because you are intentionally modifying another template modification and then that could be affected.

 

[Xon]

Do you have any situations where your template modifications in one add-on rely on modifications made in other add-ons?

I definitely have a number of add-ons where the execution order is explicitly to a non-default value to ensure compatibility between my various add-ons. This is largely used when I've got a paid add-on extending a free add-on, or for compatibility with other add-ons.

 

[XFA]

I personally see cases where I had to set execution order to a low value to ensure my template modifications where applied before other add-ons.
These cases where when the other add-on is modifying the template indenting.

I know relying on the template indenting is not necessarily a good thing but for some templates with identical code in two places but where modification should be only applied to one a regexp doesn't work, so must use template simple replacement and there is no other solution.

So I also have add-ons issues due to this change introduced to 2.1.8.

 

[l3ta]

I had to disable @Jaxel's Portal due to this bug :/

 

[Ozzy47]

So this is not a one man issue, it’s a serious problem introduced in 2.1.8

 

[Lukas W.]

Compatibility between add-ons is what we've been using execution order for exclusively too. We're having a couple of bug reports due to this too.

 

[Ozzy47]

Hopefully they come out with a patch to resolve this.

 

[Xon]

Source of the bug; https://xenforo.com/community/threa...er-and-template-modification-rebuilds.176081/

XF2.1.8 puts addon_id in these clauses at the start of the order-by list, rather than the end of the order-by list, which radically changes how these functions sort things.

The various find*ForList functions are generally per-addon grouping, having identical sorting to the get*CacheData isn't going to work too

In my opinion, the changes should have been;

For TemplateModification::applyModificationsToTemplate, the sort clause should be;

->order(['execution_order', 'addon_id', 'modification_key'])

For CodeEventListener::getListenerCacheData, the sort clause should be;

->order(['event_id', 'execute_order', 'addon_id'])

For ClassExtension::getExtensionCacheData, the sort clause should be;

->order(['from_class', 'execute_order', 'addon_id', 'to_class'])

These changes ensures deterministic sorting with arbitrary add-on install order if execution_order is the same without causing unexpected issues like the current XF2.1.8 patch does for inter-addon compatibility.

 

[kick]

In general, I don’t see any problems sculpting everyone’s priority 10 by default, and it was clear a long time ago with the extension of classes that could cause other add-ons to fail and it’s hard to debug when you have a completely different add-on. When using non-standard values, then compatibility with add-ons and not only works fine, and you can also get an error without problems.

 

[Mike]

We have fixed this and released 2.1.8 Patch 2 to resolve this.

Roughly speaking, this goes back to the 2.1.7 behavior with a couple extra sorting components to try to make sorting of these areas more deterministic. The patches are similar to what @Xon proposed above, though slightly different. (As an example, template modifications sort first by execution_order and then just modification_key, as the latter field will uniquely distinguish entries. Using addon_id is mostly a last resort.))

 

[Snog]

Will Patch 2 be rolled out immediately?

If not, could you give the exact code changes so we can test them before roll out?

 

[Chris D]

It has been released.