Refactoring template class

General discussion of development ideas and the approaches taken in the 3.x branch of phpBB. The next feature release of phpBB 3 will be 3.2/Rhea followed by 3.3.
Forum rules
Please do not post support questions regarding installing, updating, or upgrading phpBB 3.1. If you need support for phpBB 3.1 please visit the 3.1.x Support Forum on phpbb.com.

If you have questions regarding writing extensions please post in Extension Writers Discussion to receive proper guidance from our staff and community.
Post Reply
User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Refactoring template class

Post by javiexin » Fri Jan 13, 2017 3:00 pm

Hi,

I am thinking about refactoring the template class, and I would be interested in your opinions.

The reasons for refactoring are basically of two kinds: redundant code (duplicated in several parts) and inconsistency in some aspects.

Also, I would like to take a look at usability of the interface, for core and extension developers.

Key highlights:
  • Consistent use of block naming: currently, each block-level function admits maybe different syntax in the block name; as an example, alter_block_array allows for the outer[2].inner notation, while assign_block_vars only allows for outer.inner.
  • Consistent use of common code for first level blocks and nested blocks, usually unnecessarily duplicating code (more prone to errors, as it has already happened)
  • Additional functionality not currently available, as "inserting" a new block, or "deleting" a block; appending at block level (not only at the first level) and maybe more
  • Multi-level search of blocks (not only single last level)
Questions: Would this be interesting? What version should be targeted, 3.2 or 3.3? What I am planning is 100% BC, but with extended options and functionality, any feature that you would like added?

The change will basically involve major changes to the template context class, plus additions to the template interface and base class (NO BC break).
I have code quite advanced, but would like to get your opinions and suggestions.
Thanks a lot,
-javiexin

PS: For reference, these PRs are currently open that partially address this. They do not cover the whole issue though, as they are individual changes, fixing the currently documented interface, or adding small new functionality. The complete refactoring would use: a single block selection syntax, reused everywhere; a more complete set of functions; aliases to the current interface to keep BC...

User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Re: Refactoring template class

Post by javiexin » Tue Jan 31, 2017 9:24 pm

For the record, I have this ready. Any interest, or should I simply drop it, or keep it to myself?

User avatar
hanakin
Infrastructure Team
Infrastructure Team
Posts: 788
Joined: Sat Dec 25, 2010 9:02 pm
Contact:

Re: Refactoring template class

Post by hanakin » Wed Feb 01, 2017 11:35 am

javiexin

can you please elaborate what/why and maybe provide examples for those in the community on exactly this is doing in plain english as a lot of our community are not programers but more board admins/theme authors. May help get more inputs on the benefits to them.

User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Re: Refactoring template class

Post by javiexin » Wed Feb 01, 2017 2:40 pm

hanakin wrote:
Wed Feb 01, 2017 11:35 am
can you please elaborate what/why and maybe provide examples for those in the community on exactly this is doing in plain english as a lot of our community are not programers but more board admins/theme authors. May help get more inputs on the benefits to them.
Sure. And the answer is easy: this provides NOTHING to board admins and theme/style developers. They will not notice any change whatsoever.

The target of these changes are others: developers, both team members and extension writers.

Advantages for these groups:
  • Better long term supportability: as code is not duplicate across the different methods, it is easier to maintain and changes will be applied consistently (one change "fixes" all the methods in the class at once; in general, no need to replicate the change in multiple places in the file)
  • Consistency: without this change, different methods in the template class support different "blockname" specifications; with the change, ALL share the same block naming convention
  • Added functionality: besides "blockname" (string), it is possible to select blocks by "block selector" (array) that allows far more complex possibilities such as key matching at multiple levels
  • Added functionality: extra operations on blocks, such as multi-insertion (insert several blocks NOT at the end of an existing block), deletion of specific sub-blocks (not complete blocks)
  • Added functionality: variable retrieval, both at root level and at block level
And more... And I am open to suggestions and "audience requests" :) for improvement...

Of course, this is 100% backwards compatible: all interfaces that used to work, continue working in exactly the same way. But they also accept more possibilities now. And there are new methods, or new options to existing methods.

Some examples for comparison (old vs new):

Code: Select all

// Simple assign_block_vars(_array): Works now, NO changes in new implementation
$template->assign_block_vars("outer.inner",$vars);
// Complex assign_block_vars(_array), specifying intermediate block: Does not work now, works in new implementation
$template->assign_block_vars("outer[3].inner",$vars);
// Simple AND complex alter_block_array insertion (single block): works on both implementations
$template->alter_block_array("outer[3].inner", $vars, true, 'insert');
// Simple AND complex alter_block_array change: works on both implementations
$template->alter_block_array("outer[3].inner", $vars, 2, 'change');
// Using search key to find the LAST block (insert or change) in alter_block_array: Works on both implementations
$template->alter_block_array("outer[3].inner", $vars, array('NAME' => $val), 'change');
// Simple destroy_block_vars: Works now, no changes in new implementation
$template->destroy_block_vars("outer.inner");
// Complex destroy_block_vars: Does not work now, works in new implementation
$template->destroy_block_vars("outer[1].inner[0]");
Some examples of new functionality:

Code: Select all

// Retrieve var
$val = $template->retrieve_var('VARNAME');
// Retrieve block vars
$result = $template->retrieve_block_vars("outer[2].inner[]", array('VAR1', 'VAR2', 'NONEXISTENT')); 
	// $result = array('VAR1' => $val1, 'VAR2' => $val2, 'NONEXISTENT' => null);
// Block index searching at last level
$index = $template->find_key_index("outer[2].inner", array('S_ROW_INDEX' => 1)); // $index === 1
	// PR submitted for current template implementation
	// otherwise, cannot use "outer[2].inner": how would you know the value of '2'?
	// Ideally, with new implementation, this would not be needed any longer
// Multi-level block selection in ALL calls (assign_, destroy_, retrieve_block_vars, alter_block_array...)
$template->assign_block_vars(array('outer' => true, 'middle' => array('NAME', 'myname'), 'inner' => 2), $vars);
// Multi-insertion with assign_block_vars_array and alter_block_array
$template->assign_block_vars_array(array('outer' => true, 'inner' => false), $block_vars_array);
	// Inserts all blocks at the beginning of inner
// Selectively deleting with destroy_block_vars and alter_block_array
$template->alter_block_array(array('outer' => 1, 'inner' => null), array(), array('NAME' => 'to delete'), 'delete');
	// Vararray is ignored, and key in parameter list takes precedence over last block selector key, to keep BC
	// Ideally, with new implementation, this should not be used, better use the block selector array
Surely I am forgetting something... If you are interested, just ask.
-javiexin

Senky
Extension Customisations
Extension Customisations
Posts: 283
Joined: Thu Jul 16, 2009 4:41 pm

Re: Refactoring template class

Post by Senky » Sun Feb 05, 2017 12:41 pm

I like this. Wish it could be part of the core.

User avatar
3Di
Registered User
Posts: 694
Joined: Tue Nov 01, 2005 9:50 pm
Location: Milano (I) Frankfurt (D)
Contact:

Re: Refactoring template class

Post by 3Di » Mon Feb 06, 2017 1:10 am

+1, same here.

I don't see this as a new feature but a code improvement which benefits those souls involved in writing code, since will be merged. The sooner the better.
Want to compensate me for my interest? Don't ask me, just do it
I'm available for custom phpBB jobs, you can PM me the details.
New: my Live Board, Extensions and Tools for phpBB - Men at works.

User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Re: Refactoring template class

Post by javiexin » Mon Feb 06, 2017 2:28 pm

Thanks Senky, 3Di.

Just so it is not lost, I have created a PR: https://github.com/phpbb/phpbb/pull/4688
But I don't know if it will go anywhere...

-javiexin

EDIT: Passes all tests without change, so BC is 100%...

User avatar
Marc
Development Team Leader
Development Team Leader
Posts: 123
Joined: Thu Sep 09, 2010 11:36 am
Location: Munich, Germany

Re: Refactoring template class

Post by Marc » Wed Feb 08, 2017 7:22 am

We'll review any submitted PRs when we have time to do so. Sometimes other responsibilities like the work we're actually paid for looming deadlines can cause the reviews to take a bit longer or being done later we'd also like to. ;)

User avatar
javiexin
Registered User
Posts: 90
Joined: Thu Dec 22, 2011 10:04 am

Re: Refactoring template class

Post by javiexin » Wed Feb 08, 2017 10:31 am

Thanks a lot Marc. I can only be but thankful to all the work that the DevTeam and the rest of the phpbb staff volunteer to this, so I perfectly understand priorities.

With that in mind is why I proposed the changes BEFORE pushing the PR, because this is something that you may have discarded in the past, and I wouldn't know, or simply does not fit with the evolution you may have in mind for phpbb. Not receiving any feedback is what made me publish the PR so that the code is not lost, and you can review it when you have time/want.

Anyway, as I said, thanks a lot for all your time, effort and expertise that is put to serve this community!
Best regards,
-javiexin

PS: I currently have 12 open PRs, I think it is the (current) record :)

Post Reply