Refactoring template class

General discussion of development ideas and the approaches taken in the 3.x branch of phpBB. The current feature release of phpBB 3 is 3.3/Proteus.
Forum rules
Please do not post support questions regarding installing, updating, or upgrading phpBB 3.3.x. If you need support for phpBB 3.3.x please visit the 3.3.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 »

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 »

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

User avatar
hanakin
Front-End Dev Team Lead
Front-End Dev Team Lead
Posts: 968
Joined: Sat Dec 25, 2010 9:02 pm
Contact:

Re: Refactoring template class

Post by hanakin »

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.
Donations welcome via Paypal Image

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

Re: Refactoring template class

Post by javiexin »

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: 315
Joined: Thu Jul 16, 2009 4:41 pm

Re: Refactoring template class

Post by Senky »

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

User avatar
3Di
Registered User
Posts: 951
Joined: Tue Nov 01, 2005 9:50 pm
Location: Milano 🇮🇹 Frankfurt 🇩🇪
Contact:

Re: Refactoring template class

Post by 3Di »

+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.
🆓 Free support for our extensions also provided here: phpBB Studio
🚀 Looking for a specific feature or alternative option? We will rock you!
Please PM me only to request paid works. Thx. Want to compensate me for my interest? Donate
My development's activity º PhpStorm's proud user º Extensions, Scripts, MOD porting, Update/Upgrades

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

Re: Refactoring template class

Post by javiexin »

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: 185
Joined: Thu Sep 09, 2010 11:36 am
Location: Munich, Germany

Re: Refactoring template class

Post by Marc »

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 »

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