Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ticket/15905] Create multiple twig extensions #5480

Open
wants to merge 8 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@mrgoldy
Copy link
Contributor

commented Dec 9, 2018

PHPBB3-15905

Checklist:

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

Tracker ticket (set the ticket ID to your ticket ID):

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

mrgoldy added some commits Dec 9, 2018

@3D-I

This comment has been minimized.

Copy link
Contributor

commented Dec 10, 2018

So what's the issue wtith the auth?

@mrgoldy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

Unfortunately that service is not available in some travis tests.
I've tried various ways, through using AbstractExtension and through using a RuntimeLoaderInterface, neither seem to solve the issue.
I am still able to use the simple function with a permission and get the corresponding value (true/false). But the tests do not pass, so I am obviously missing something here. But I do not know what/where, ergo can not create it.

not sure what I expected though, as ruben ran into the same issue: #5413

@rubencm

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

I think thats why i told you to not look at my last commit.
Auth service don't exist in installer environment, you have to add those services in other file

@mrgoldy

This comment has been minimized.

Copy link
Contributor Author

commented Dec 10, 2018

Yeah, I know. But that would mean we would end up with several twig extensions in one place, and several in an other. Isn't that confusing?

@rubencm

This comment has been minimized.

Copy link
Member

commented Dec 10, 2018

Yes, or just don't include the default services_twig.yml in installer environment and use ti's own, don't know, I moved it to another file, but probably there are better ways to do it

@hanakin

This comment has been minimized.

Copy link
Member

commented Mar 9, 2019

Just seen this can we rework this to operate similar to how you did for the icons with html templates for the username and avatars? The idea would be for these and the ones for the icons to be contained in a sub folder called macros or something along those lines. This will go along way ay to removing all the html from the backend into the front-end

@mrgoldy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 12, 2019

well yes, that is possible. But currently the twig function I created here is just a 'wrapper' for the phpbb_get_user_avatar function. I do not alter that function at all.
I could instead close this pr / turn it into reworking the get_username_string and phpbb_get_user_avatar functions.

Only thing I am uncertain / worried about is calling $template->assign_display() - or any other rendering method - so many times per page. I am uncertain how that would impact loading times and I do not know a solid way of testing it..

@hanakin

This comment has been minimized.

Copy link
Member

commented Mar 12, 2019

Why would you I’m not saying it should be called in the function. It should still work the same way and create variables or even one variable array that is passed to the function in the template. The function only needs the if else/switch logic based on what’s passed to serve the correct html file.

Also we have decided to use Pascal Case for twig functions that return template code. So lang will saty as is but if it returns html tags it should use Pascal Casing.

@mrgoldy

This comment has been minimized.

Copy link
Contributor Author

commented Mar 16, 2019

Then I am not sure what you're asking here.
I thought you mean similar to the "function", create the variables and then render a new template file (eg. username.html). This so that the hard-coded template strings from get_username_string() are gone.
Which is possible, but that would mean I have to re-write the respective function, or we end up with two functions for usernames.. One that's called from PHP and one that's caleld from TWIG. Which is bad in my opinion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.