[ticket/15214] Add event & functionality for assigning template event priority #6571

toxyy · 2023-12-03T21:36:09Z

Event added to allow template events to be assigned priority per extension, event location chosen so that it only fires once.
Twig node event class refactored to allow template event priority assignment, compile calls are deferred until all locations are processed per extension namespace.
Priority precedence mirrors Symfony priority, with higher numbers being placed at the beginning of the array.
Duplicate priority assignment will currently have the later events compiled before the others.

PHPBB3-15214

Checklist:

Correct branch: master for new features; 3.3.x for fixes
Tests pass
Code follows coding guidelines: master and 3.3.x
Commit follows commit message format

Tracker ticket:

https://tracker.phpbb.com/browse/PHPBB3-15214

… priority Event added to allow template events to be assigned priority per extension, event location chosen so that it only fires once. Twig node event class refactored to allow template event priority assignment, compile calls are deferred until all locations are processed per extension namespace. Priority precedence mirrors Symfony priority, with higher numbers being placed at the beginning of the array. Duplicate priority assignment will currently have the later events compiled before the others. PHPBB3-15214

toxyy · 2023-12-03T21:39:33Z

Example event usage: (unsure about the name)

public function twig_tokenparser_constructor($event)
{
	$template_event_priority_array = $event['template_event_priority_array'];
	$template_event_priority_array += [
		'toxyy_postformtemplates' => [
			'event/overall_header_head_append' => 1000
		]
	];
	$event['template_event_priority_array'] = $template_event_priority_array;
}

With this, my template event is called before any others. Normal behavior would have it placed on the bottom of this list.

As stated in the commit message, priority works like in Symfony events, the higher number will be compiled first.

If two events have the same priority, then the last one added will be executed last - this differs from what's stated in my initial commit message, sorry about that, I was thinking about it wrong.

Reason being is that for an array:

[
  '0' => 'first_event',
  '1' => 'second_event',
  '2' => 'third_event',
  '3' => 'fourth_event'
]

if your priority is 2, then it will be placed as so:

[
  '0' => 'first_event',
  '1' => 'second_event',
  '2' => 'priority_set_event',
  '3' => 'third_event',
  '4' => 'fourth_event'
]

And then if another event has a priority of 2, then it will be placed above it:

[
  '0' => 'first_event',
  '1' => 'second_event',
  '2' => 'another_priority_set_event',
  '3' => 'priority_set_event'.
  '4' => 'third_event',
  '5' => 'fourth_event'
]

And because the higher numbers get executed first (krsort), it will get compiled after the previously set template event for the same priority. This is an edge case though. Not sure how Symfony handles this. Template events by default are parsed in alphabetical order by extension namespace. If no one uses the event, then template events will execute in the same order as they do now.

Add new dispatch parameter to the template\twig\extension call PHPBB3-15214

Add new dispatch parameter to the template\twig\extension calls PHPBB3-15214

Arrow functions aren't added until PHP 7.4, so we can't use them yet. Anonymous functions have been added since PHP 5.3 PHPBB3-15214

toxyy · 2023-12-03T23:01:22Z

I don't know if the compiler object's write methods change the state of the compiler object itself. If so then $compiler should be passed as &$compiler in the use() of the anonymous functions, but as it's currently written, it works fine.

Also unsure if it's necessary to set $phpbb_dispatcher to null within phpbb\template\twig\tokenparser\event on this line and check for that on this line, but the null check seems to be standard for most other classes in the template\twig folder. Even though the constructor is only being called in one place in the entire codebase

Also unsure about proper code formatting for the anonymous functions, I don't have a phpstorm license so I can't run it through that to check, and there are no anonymous functions as far as I can find within core so I'm not sure if they're accounted for with code styling.

Hopefully this is fine on the 3.3 branch? 😄

It doesn't need to be set at the top and makes more sense to set it at the bottom where it's used. Also removes a redundant check PHPBB3-15214

phpBB/phpbb/template/twig/node/event.php

rxu · 2023-12-05T07:09:02Z

phpBB/phpbb/template/twig/tokenparser/event.php

+			extract($this->phpbb_dispatcher->trigger_event('core.twig_tokenparser_constructor', compact($vars)));
+		}
+
+		$this->template_event_priority_array = $template_event_priority_array;


It probably makes sense to unset($template_event_priority_array); as it's not used anymore.

Adds similar usage examples like the event core.permissions has PHPBB3-15214

toxyy · 2023-12-05T08:27:48Z

All I did was change what's within a docblock comment... that test should not have failed

Make docblock look a bit cleaner and restart the tests PHPBB3-15214

rxu · 2023-12-05T08:45:55Z

That's random network issue, unrelated.

toxyy · 2023-12-05T08:46:57Z

No matter, I think the format of this docblock update is cleaner anyways

phpBB/phpbb/template/twig/node/event.php

rxu · 2023-12-09T05:08:24Z

To sum my all above comments, I feel like simplified all the stuff and below code works just fine.

	public function compile(\Twig\Compiler $compiler)
	{
		$compiler->addDebugInfo($this);

		$location = $this->listener_directory . $this->getNode('expr')->getAttribute('name');

		$template_events = [];
		foreach ($this->environment->get_phpbb_extensions() as $ext_namespace => $ext_path)
		{
			$ext_namespace = str_replace('/', '_', $ext_namespace);
			$priority_key = $this->template_event_priority_array[$ext_namespace][$location] ?? 0;
			$template_events[$priority_key][] = $ext_namespace;
		}
		krsort($template_events);

		foreach ($template_events as $priority => $events)
		{
			foreach ($events as $ext_namespace)
			{
				if ($this->environment->isDebug() || $this->environment->getLoader()->exists('@' . $ext_namespace . '/' . $location . '.html'))
				{
					if ($this->environment->isDebug())
					{
						// If debug mode is enabled, lets check for new/removed EVENT
						//  templates on page load rather than at compile. This is
						//  slower, but makes developing extensions easier (no need to
						//  purge the cache when a new event template file is added)
						$compiler
							->write("if (\$this->env->getLoader()->exists('@{$ext_namespace}/{$location}.html')) {\n")
							->indent();
					}

					$compiler
						->write("\$previous_look_up_order = \$this->env->getNamespaceLookUpOrder();\n")

						// We set the namespace lookup order to be this extension first, then the main path
						->write("\$this->env->setNamespaceLookUpOrder(array('{$ext_namespace}', '__main__'));\n")
						->write("\$this->env->loadTemplate('@{$ext_namespace}/{$location}.html')->display(\$context);\n")
						->write("\$this->env->setNamespaceLookUpOrder(\$previous_look_up_order);\n");

					if ($this->environment->isDebug())
					{
						$compiler
							->outdent()
							->write("}\n\n");
					}
				}
			}
		}
	}

rxu · 2023-12-09T18:05:03Z

I guess the issue with the commit messages here is an empty line needed between the explanation line and PHPBB3-15214 line.

toxyy · 2023-12-09T18:06:34Z

I guess the issue with the commit messages here is an empty line needed between the explanation line and PHPBB3-15214 line.

There are also some other failures like here:
https://github.com/phpbb/phpbb/actions/runs/7152664064/job/19478240191?pr=6571#step:10:1060

I'll do a force push to fix the messages in a bit.

…tests PHPBB3-15214

PHPBB3-15214

Putting this here will have the loop run the same amount of times as default. Without this, the loop runs too many extra times. PHPBB3-15214

toxyy · 2023-12-09T18:55:04Z

@rxu Yeah looks like it doesn't like the added tests right now

I'm guessing that it's because template_event_order.php and template_event_order.php have the same class name for foo\bar and the same for template_event_order.php and template_event_order_lower.php for foo\foo. Unsure though. Maybe not. I've tried to look through everything else and I don't understand why it's failing.

Added this when I initially tested the if block move. but I forgot to add it in the actual commit. If this isn't moved, no template events work, and the tests fail. PHPBB3-15214

rxu · 2023-12-10T02:24:55Z

That's weird as everything works when installing both of the test extensions on a local test board and yaml for both of services.yml is valid.

EDIT: I guess I've got what's the issue, let me test something.

PHPBB3-15214

toxyy · 2023-12-10T06:56:07Z

@rxu That solved the errors we were seeing before, but we've got a new set of failures 😓

rxu · 2023-12-10T06:57:31Z

I'm afraid Win+PHP 8.3 errors aren't related to this PR. Will dig into it a bit.

toxyy · 2023-12-10T06:58:08Z

Tests passed earlier in this PR so I'm not sure what changed.

toxyy · 2023-12-10T07:06:48Z

Also, I've been thinking.

Currently, all events are added to the same priority of 0 in the order that they would originally be run. I'm not sure how Symfony events work exactly, but would there be any scenario in which someone would want to place a template event in the middle of those? Technically, that would mean that they cannot all be at priority key 0. That is also installation dependent, as the priority keys would change based on what extensions you have enabled, assuming multiple extensions use the same events.

Would it be beneficial to somehow make it so you could say "if extension X is installed, make my template event compile before theirs"? That could be solved with a priority key, yes, but lets say you also want all other extensions to execute before yours as well - meaning you specifically want your template event to compile right before extension X's event of the same name (though anyone else doing the same would run into default behavior, of course).

Let's take navlinks for example. Maybe you want to place your link right before the other nav links of extension X, but you don't want it to be the first links added. With a priority key, the only way to get it before extension X would be to make it compile first, meaning your links would be first. That doesn't seem versatile.

Is what I'm explaining making sense, and is that a valid use case to try to add functionality for?

rxu · 2023-12-10T07:15:18Z

I'm not sure I fully got the point in question.
With your PR you're able to set priority for any other extension template event (by other authors, unlike core events). So if you want to change, f.e., quick links extensions order with your extension you just set priority keys as you wish for another extension(s) template events.

toxyy · 2023-12-10T07:16:39Z

Ah yes you could do that. I guess that case would be solved then.

rxu · 2023-12-10T07:40:45Z

And the error is:

PHP FATAL ERROR:  UNCAUGHT TWIG\ERROR\LOADERERROR: UNABLE TO FIND TEMPLATE "@FOO_BAR/EVENT/NAVBAR_HEADER_QUICK_LINKS_AFTER.HTML" (LOOKED INTO: ) IN "NAVBAR_HEADER.HTML" AT LINE 75. IN D:\A\PHPBB3\PHPBB3\PHPBB\VENDOR\TWIG\TWIG\SRC\LOADER\FILESYSTEMLOADER.PHP

D:\a\phpbb3\phpbb3\tests\test_framework\phpbb_functional_test_case.php:983
D:\a\phpbb3\phpbb3\tests\test_framework\phpbb_functional_test_case.php:143
D:\a\phpbb3\phpbb3\tests\test_framework\phpbb_functional_test_case.php:807
D:\a\phpbb3\phpbb3\tests\functional\notification_test.php:46

That's weird again as this template event shouldn't be involved there.

toxyy · 2023-12-10T07:54:59Z

If that said that it was for @FOO_FOO I'd say to also add foo/foo/styles/prosilver/template/ to $fixtures but I'm at a loss currently.

Why is it looking in /event/ for the template?

rxu · 2023-12-10T10:52:48Z

The issue revealed by this PR is the twig compiled cache folder was never purged in Windows test environment. Will send a fix PR to this PR soon.

Purge Twig compiled cache in Windows. Set appropriate folder access control options to do that. PHPBB3-15214

toxyy · 2023-12-10T16:03:46Z

Failed asserting that 'PHP FATAL ERROR: UNCAUGHT TWIG\ERROR\LOADERERROR: THERE ARE NO REGISTERED PATHS FOR NAMESPACE "FOO_BAR" IN "NAVBAR_HEADER.HTML" AT LINE 75. IN D:\A\PHPBB\PHPBB\PHPBB\VENDOR\TWIG\TWIG\SRC\LOADER\FILESYSTEMLOADER.PHP:227

😄

rxu · 2023-12-10T16:11:48Z

Hmm. Tests were passing successfully here https://github.com/rxu/phpbb3/actions/runs/7157430146 on Win+PHP 8.3 with this code. One more weirdness :) Okay, will look into it furher.

rxu · 2023-12-10T16:44:45Z

@toxyy Could you try adding these 3 lines to .github/workflows/tests.yml rxu@0e2b349#diff-1db27d93186e46d3b441ece35801b244db8ee144ff1405ca27a163bfe878957fR171-R173
Tests pass here https://github.com/rxu/phpbb3/actions/runs/7158983020

Adding per rxu's recommendation PHPBB3-15214

toxyy · 2023-12-10T17:02:46Z

@rxu same error 😄
Failed asserting that 'PHP FATAL ERROR: UNCAUGHT TWIG\ERROR\LOADERERROR: THERE ARE NO REGISTERED PATHS FOR NAMESPACE "FOO_BAR" IN "NAVBAR_HEADER.HTML" AT LINE 75. IN D:\A\PHPBB\PHPBB\PHPBB\VENDOR\TWIG\TWIG\SRC\LOADER\FILESYSTEMLOADER.PHP:227

rxu · 2023-12-10T17:09:05Z

Nice.

EDIT: the problem is that twig cache folder is not being purged under Windows for some reason. I'm out of ideas for now.

toxyy · 2023-12-10T19:09:15Z

@marc1706 maybe you have some ideas? Not sure who else has worked with the tests a lot

PHPBB3-15214

toxyy · 2023-12-12T14:45:51Z

@rxu It worked this time 😄 thanks for the help

rxu · 2023-12-12T15:25:29Z

Great. The issue is that after the purge_cache() call tests go forward fast so that the next one got executed before all cache files are finished to delete. Hence the error. sleep() solves it.

marc1706 · 2024-05-04T07:25:09Z

As noted on discord, the PR might result in BC breaks for some and adds quite a bit of changes to behavior and introduces new functionality so this shouldn't aim for 3.3.x but rather 4.0.

rxu · 2024-05-04T07:31:29Z

@toxyy mind to rebase vs master?

toxyy · 2024-05-07T15:11:24Z

@rxu there are a lot of conflicts that I'll need to go through, but sure. I also need to change the event's since version.

toxyy added 2 commits December 3, 2023 16:52

[ticket/15214] Test fix for test_helper_url_no_rewrite

12fd385

Add new dispatch parameter to the template\twig\extension call PHPBB3-15214

[ticket/15214] Test fix for test_bbcode_firstpass

2c1e43a

Add new dispatch parameter to the template\twig\extension call PHPBB3-15214

toxyy force-pushed the ticket/15214 branch from 1eaad5c to 2c1e43a Compare December 3, 2023 22:20

toxyy added 2 commits December 3, 2023 17:30

[ticket/15214] Add fixes for various other tests

c7d16fe

Add new dispatch parameter to the template\twig\extension calls PHPBB3-15214

[ticket/15214] Replace arrow functions with anonymous functions

8d6d016

Arrow functions aren't added until PHP 7.4, so we can't use them yet. Anonymous functions have been added since PHP 5.3 PHPBB3-15214

[ticket/15214] Move setting $priority_key to a more appropriate place

31fb27e

It doesn't need to be set at the top and makes more sense to set it at the bottom where it's used. Also removes a redundant check PHPBB3-15214

rxu reviewed Dec 5, 2023

View reviewed changes

phpBB/phpbb/template/twig/node/event.php Outdated Show resolved Hide resolved

rxu reviewed Dec 5, 2023

View reviewed changes

[ticket/15214] Provide usage example within event docblock

e1f597c

Adds similar usage examples like the event core.permissions has PHPBB3-15214

[ticket/15214] Update block, restart tests

64b9aa2

Make docblock look a bit cleaner and restart the tests PHPBB3-15214

rxu reviewed Dec 8, 2023

View reviewed changes

phpBB/phpbb/template/twig/node/event.php Outdated Show resolved Hide resolved

rxu reviewed Dec 8, 2023

View reviewed changes

phpBB/phpbb/template/twig/node/event.php Outdated Show resolved Hide resolved

rxu reviewed Dec 9, 2023

View reviewed changes

phpBB/phpbb/template/twig/node/event.php Outdated Show resolved Hide resolved

rxu added 2 commits December 9, 2023 13:10

[ticket/15214] Optimize event node code and add template event order …

9603924

…tests PHPBB3-15214

[ticket/15214] Fix test foo/foo extension listener

1e2532a

PHPBB3-15214

toxyy force-pushed the ticket/15214 branch from 4bc0fba to 1e2532a Compare December 9, 2023 18:14

[ticket/15214] Move if statement to a better place

648cd68

Putting this here will have the loop run the same amount of times as default. Without this, the loop runs too many extra times. PHPBB3-15214

[ticket/15214] Move $ext_namespace str_replace to work

3714202

Added this when I initially tested the if block move. but I forgot to add it in the actual commit. If this isn't moved, no template events work, and the tests fail. PHPBB3-15214

[ticket/15214] Fix tests

96ce8e7

PHPBB3-15214

[ticket/15214] Fix Windows tests

50d4e6b

Purge Twig compiled cache in Windows. Set appropriate folder access control options to do that. PHPBB3-15214

[ticket/15214] Fix tests again

d38d59c

Adding per rxu's recommendation PHPBB3-15214

[ticket/15214] Fix Windows tests

01ae4ed

PHPBB3-15214

toxyy changed the base branch from 3.3.x to master May 7, 2024 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ticket/15214] Add event & functionality for assigning template event priority #6571

[ticket/15214] Add event & functionality for assigning template event priority #6571

toxyy commented Dec 3, 2023 •

edited

toxyy commented Dec 3, 2023 •

edited

toxyy commented Dec 3, 2023 •

edited

rxu Dec 5, 2023

toxyy commented Dec 5, 2023

rxu commented Dec 5, 2023 •

edited

toxyy commented Dec 5, 2023

rxu commented Dec 9, 2023 •

edited

rxu commented Dec 9, 2023

toxyy commented Dec 9, 2023

toxyy commented Dec 9, 2023 •

edited

rxu commented Dec 10, 2023 •

edited

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023

toxyy commented Dec 10, 2023

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023 •

edited

toxyy commented Dec 10, 2023 •

edited

rxu commented Dec 10, 2023 •

edited

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023

rxu commented Dec 10, 2023

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023 •

edited

toxyy commented Dec 10, 2023

toxyy commented Dec 12, 2023

rxu commented Dec 12, 2023

marc1706 commented May 4, 2024

rxu commented May 4, 2024

toxyy commented May 7, 2024

[ticket/15214] Add event & functionality for assigning template event priority #6571

Are you sure you want to change the base?

[ticket/15214] Add event & functionality for assigning template event priority #6571

Conversation

toxyy commented Dec 3, 2023 • edited

toxyy commented Dec 3, 2023 • edited

toxyy commented Dec 3, 2023 • edited

rxu Dec 5, 2023

Choose a reason for hiding this comment

toxyy commented Dec 5, 2023

rxu commented Dec 5, 2023 • edited

toxyy commented Dec 5, 2023

rxu commented Dec 9, 2023 • edited

rxu commented Dec 9, 2023

toxyy commented Dec 9, 2023

toxyy commented Dec 9, 2023 • edited

rxu commented Dec 10, 2023 • edited

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023

toxyy commented Dec 10, 2023

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023 • edited

toxyy commented Dec 10, 2023 • edited

rxu commented Dec 10, 2023 • edited

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023

rxu commented Dec 10, 2023

toxyy commented Dec 10, 2023

rxu commented Dec 10, 2023 • edited

toxyy commented Dec 10, 2023

toxyy commented Dec 12, 2023

rxu commented Dec 12, 2023

marc1706 commented May 4, 2024

rxu commented May 4, 2024

toxyy commented May 7, 2024

toxyy commented Dec 3, 2023 •

edited

toxyy commented Dec 3, 2023 •

edited

toxyy commented Dec 3, 2023 •

edited

rxu commented Dec 5, 2023 •

edited

rxu commented Dec 9, 2023 •

edited

toxyy commented Dec 9, 2023 •

edited

rxu commented Dec 10, 2023 •

edited

rxu commented Dec 10, 2023 •

edited

toxyy commented Dec 10, 2023 •

edited

rxu commented Dec 10, 2023 •

edited

rxu commented Dec 10, 2023 •

edited