[ticket/12224] Add template wrapper method to assign block arrays #2056

Merged
merged 2 commits into from Mar 10, 2014

3 participants

@rxu

Add one more wrapper template method for the function assign_block_vars() which
takes a 2-dimensional array as a parameter
and calls assign_block_vars() in a loop to assign the whole block loop at once.
This can make the core a little bit more expendable from the point of
developing extensions as it allows to pass the data to events before
it's being assigned to template.

PHPBB3-12224.

@rxu rxu [ticket/12224] Add template wrapper method to assign block arrays
Add one more wrapper template method for the function assign_block_vars() which
takes a 2-dimensional array as a parameter
and calls assign_block_vars() in a loop to assign the whole block loop at once.
This can make the core a little bit more expendable from the point of
developing extensions as it allows to pass the data to events before
it's being assigned to template.

PHPBB3-12224
dda775c
@bantu
phpBB Forum Software member

Unit tests please

@nickvergessen
phpBB Forum Software member

From ticket:

But your event will be triggered on any page call, for every loop we have. And you only get the array with data to identify what to do with it.
I don't think its helpful. We should just add events to allow modifying the arrays before we set them.

I really doubt the use of this

@rxu

Test added.

@nickvergessen
phpBB Forum Software member

this does not work for nested stuff like forum_row and subforum_row does it?

@rxu

@nickvergessen I'm not sure what do you mean, it works exactly like it looks. For example, it can assign subforum_row within forum_row loop as 'forumrow.subforumrow' nested block taking subforum_row array of arrays of template variables as an argument.

@nickvergessen nickvergessen commented on an outdated diff Feb 25, 2014
tests/template/template_test.php
+ );
+
+ return array(
+ array('outer.middle', $block_vars_array),
+ );
+ }
+
+ /**
+ * @dataProvider assign_block_vars_array_data
+ */
+ public function test_assign_block_vars_array($blockname, array $block_vars_array)
+ {
+ $this->template->set_filenames(array('test' => 'loop_nested.html'));
+
+ // Initialize wrapper block for the nested loop first
+ $this->template->assign_block_vars('outer', array('VARIABLE' => 'Test assigning block vars array loop:'));
@nickvergessen
phpBB Forum Software member

can we test this in assign_block_vars_array aswell?
so we have normal loops and nested loops in tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@nickvergessen nickvergessen commented on the diff Mar 1, 2014
phpBB/phpbb/template/context.php
@@ -198,6 +198,22 @@ public function assign_block_vars($blockname, array $vararray)
}
/**
+ * Assign key variable pairs from an array to a whole specified block loop
+ *
+ * @param string $blockname Name of block to assign $block_vars_array to
+ * @param array $block_vars_array An array of hashes of variable name => value pairs
+ */
+ public function assign_block_vars_array($blockname, array $block_vars_array)
+ {
+ foreach ($block_vars_array as $vararray)
+ {
+ $this->assign_block_vars($blockname, $vararray);
+ }
+
+ return true;
@nickvergessen
phpBB Forum Software member

is this return needed? if so, it should also be able to be false.
And it should be added to the doc block

@nickvergessen
phpBB Forum Software member

Also doc block does not match the doc block from the abstract class

@rxu
rxu added a note Mar 2, 2014

@nickvergessen As for return, this is similar assign_block_vars() function. TBH, I don't know why is it needed but desided to follow the existing code because it was possibly discussed somewhere.
As for the docblock mismatch to abstract class, it can't match because the return is different for abstract. Description for the function and parameters does match for both.

@nickvergessen
phpBB Forum Software member

Well the idea of abstract is that it shows what the method does.
So either you return $this; (prefered) or you change the abstracts return to bool.

If this is the same on assign_block_vars, that one needs fixing in another ticket

@rxu
rxu added a note Mar 2, 2014

This is not assign_block_vars() only, but assign_var(), append_var() and others ;) Just take a look at template.php and context.php and compare. So I followed the existing status for template class.

So, what am I supposed to do? Change abstract class definition docblock for return to bool?

@rxu
rxu added a note Mar 2, 2014

Actually, I'd rather got rid of return here at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@rxu
rxu commented Mar 2, 2014

As for the returning $this, it retirned within phpBB/phpbb/template/base.php.
So, I wonder if it should be returned twice.

Moreover, class "context" doesn't implement abstract class "template", so dockblocks shouldn't match at all. Class "context" is implemented by class "base" in base.php.

Thus, I wonder if the last commit should be reverted here.

@nickvergessen
phpBB Forum Software member

@rxu you are right, i thought base is the abstract :/
So i guess removing the return is fine. doc block should say return null

@rxu
rxu commented Mar 3, 2014

@nickvergessen Updated.

@rxu rxu [ticket/12224] Add assign_block_vars_array() test.
Add tests, also change return value to null.

PHPBB3-12224
d29514f
@nickvergessen nickvergessen merged commit d29514f into phpbb:develop Mar 10, 2014

1 check passed

Details default The Travis CI build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment