Add a node with data about the user avatar to the output (alternative)

[dragunoff]

An alternative implementation of #48 that doesn't use regex scraping but a simplified version of phpbb's avatar function.

Closes #46

[dragunoff]

Update: addressed comments from the review.

[dragunoff]

Update: add a method to handle local (gallery) avatars.

[deleted-user-1]

TBH, now that you are hardcoding the paths here too and rely on the implementation type ('avatar.driver.x), I don't really understand why you still want to copy these phpBB code fragments instead of doing it in a custom but simple way similar to what I already suggested with https://github.com/OpenRA/openrauseraccounts/commit/5806e2abd86d2835bbb5d59481e64eb657e1729e (just add the width and height like you did here). I don't really care but don't see the profit tbh.

[dragunoff]

@matjaeck It all started by copying straight from phpBB's implementation with the intention to do it the "phpBB way" as I wasn't familiar with the software. I can see how it works now so I won't be against simplifying our end.

[deleted-user-1]

@matjaeck It all started by copying straight from phpBB's implementation with the intention to do it the "phpBB way"

I remember that sentence 😄

Anyway, I hope the comments have been helpful.

[dragunoff]

Update: addressed https://github.com/OpenRA/openrauseraccounts/pull/49/files#r550792886

[deleted-user-1]

Update: addressed https://github.com/OpenRA/openrauseraccounts/pull/49/files#r550792886

Hm, I might have confused you with that comment about the config file. My point was that the arguments (I think) can be removed there, when fetching services from a container, but it was not related to this PR. So I'm not sure why you changed it now to inject the services in the constructor. I think both is fine, however since this file is already using the phpbb_container for some services, I'd say use this method for all of them consistently (i.e. fetch config and db as you did before and perhaps remove the unneeded argument from the service file in another PR or so).

[dragunoff]

Hm, I might have confused you with that comment about the config file. My point was that the arguments (I think) can be removed there, when fetching services from a container, but it was not related to this PR. So I'm not sure why you changed it now to inject the services in the constructor. I think both is fine, however since this file is already using the phpbb_container for some services, I'd say use this method for all of them consistently (i.e. fetch config and db as you did before and perhaps remove the unneeded argument from the service file in another PR or so).

@matjaeck I think I see now :thinking: either way works fine but it's just inconsistent across the code. Are you suggesting to get everything from global $phpbb_container and remove the injections?

[deleted-user-1]

I think I see now 🤔 either way works fine but it's just inconsistent across the code. Are you suggesting to get everything from global $phpbb_container and remove the injections?

For this PR yes, to make it consistent in this file at least. Another PR could remove the argument @dbal.conn from the configuration for the openra.openrauseraccounts.core service in services.yml. Why is it not needed?

Because phpbb already set their service container (phpbb_container) up to include the dbal.conn service. So we do not need to use constructor injection to access the service in our class but can fetch and use the service by accessing the service container, e.g. container->get('service_id').

You can fetch all phpbb-services from the phpbb_container. The global keyword is needed to access the variable $phpbb_container (which holds the reference to the service container), which was declared (somewhere) outside our scope, in global variable scope.

We can also register our own classes as services and then fetch them from the phpbb_container. For example, we would still need to include the following in services.yml:

    openra.openrauseraccounts.core:
        class: openra\openrauseraccounts\core\core
        arguments: []

That is because we want to access the openra.openrauseraccounts.core service from other places in the code, like we do in the UCP module.

---

I'm not sure if there is any benefit for us of using constructor injection over registering and fetching services from phpbb's container. I tend to believe that everything could use fetching, but then this is all based on an outdated version of phpbb and symfony. I also think to remember that phpbb modules (UCP, ACP,...) can not use constructor injection for some reason. Phpbb's codebase is rather confusing and inconsistent so it's hard to find the "phpbb-way"-answer here.

I noticed while reading through this again that when fetching services from a container, the official examples do not use class properties but simple variables (no private <property_name> in https://symfony.com/doc/2.8/service_container.html#fetching-and-using-services, however properties are used when using constructor injection (https://symfony.com/doc/2.8/service_container.html#injecting-services-config-into-a-service). This could mean that the private properties in the modules are not needed and that we could(should?) use variables there.

---

The best way I can imagine to update the extension to the correct patterns is to

• update phpbb to its latest version
• check out which symfony version it's supporting now
• check out how the latest skeleton extension handles these things in the examples for modules and controllers (and in general)
• update the codebase accordingly to what the symfony docs suggest and how it's done in the skeleton extension.

This PR however should focus on extending the API without trying to improve too many other things.

[pchote]

The forum is already running the 3.3.x series, which afaik is the lastest public branch.

[dragunoff]

Update: revert to fetching the sevrices and config from the phpbb container and fixed function documentation.

Thanks for the explanation @matjaeck

[dragunoff]

Update: addressed style nits by @pchote

[pchote]

Something is wrong with the absolute url generation.

https://forum.openra.net/openra/info/3bbaf407faffb233b39bfd899b8e4710add2311b → https://forum.openra.net/download/file.php?avatar=2_1527436933.gif ✔️

https://forum.openra.net/openra/info/1dcfb98fee701365563c26a87407356b6bfabd27 → ./../../download/file.php?avatar=6680.jpg :x:

[ubitux]

This is what I observe:

c46d11959601de8dfb3ac0b43d11f3913c66e394|6680|Clockwork|./../../download/file.php?avatar=6680.jpg
18f9dc5ba25e87952c1d243bd43a72682ce8d85b|7813|TiTo|https://forum.openra.net/download/file.php?avatar=7813_1567075730.jpg
0d327f1d1bca986a762234ce551d448d389f9e0b|7223|Punsho|https://forum.openra.net/download/file.php?avatar=7223_1532567908.jpg
a843fce0525a173c66b21c29fbd14a6192d371b7|3952|anjew|./../../download/file.php?avatar=3952.png
fee5b0ea94f717f66dcfee4e92e7f9c2187e29f5|6723|WhoCares|./../../download/file.php?avatar=6723.jpg
791cc59c91395d1b73b77d0470c4a90ad3ecf0ae|6856|netnazgul|./../../download/file.php?avatar=6856.jpg
ec693acee2494eeb4598e695fcd5e749ce5d18e3|2580|KOYK_GR|./../../download/file.php?avatar=2580.gif
a775c2a77a87991f56e563bddad61ed65cb6828c|6723|WhoCares|./../../download/file.php?avatar=6723.jpg
e6c863880257a942afe62a575b2fc0eba0d6ef99|7304|Upps|https://forum.openra.net/download/file.php?avatar=7304_1538927084.png
5196cc97eb3c6fc81023160e778aa2ad684d6225|6016|Fahrrad|https://forum.openra.net/download/file.php?avatar=6016_1543068237.jpg
ccf527f619e30326e5d7f38b19dbd2bf54ced5ec|6748|Orb|./../../download/file.php?avatar=6748.png
ff658f81acb394518762f4ac07a61edd6e57abe7|7304|Upps|https://forum.openra.net/download/file.php?avatar=7304_1538927084.png
1dcfb98fee701365563c26a87407356b6bfabd27|6680|Clockwork|./../../download/file.php?avatar=6680.jpg
ecc1e74d05307f17ba83585bc093840befea05e0|6680|Clockwork|./../../download/file.php?avatar=6680.jpg
8a27f6f670012b24761b9a8f9bb3655c6905402d|6504|JuiceBox|./../../download/file.php?avatar=6504.jpg
10b4ac8f4fd4acee69705aa495865ad774a39e61|7714|Nilhall|https://forum.openra.net/download/file.php?avatar=7714_1586245660.jpg
08e5ad0169ea41966922875ab6cd4d85db409ddc|7226|Super_Newbie|https://forum.openra.net/download/file.php?avatar=7226_1553730875.png
5067b0653fb05bd666e50e941a0a4c922c99efed|4527|FiveAces|https://forum.openra.net/download/file.php?avatar=4527_1538325636.gif
0d1317d6c8fa92a57d45be733f225ba399cd6a3d|6016|Fahrrad|https://forum.openra.net/download/file.php?avatar=6016_1543068237.jpg
c41eafa44d71b05f397af7eb0d613f67dca1099a|6680|Clockwork|./../../download/file.php?avatar=6680.jpg

Columns: fingerprint, profile ID, profile Name, avatar URL

[pchote]

Apparently 1 is a valid user_avatar_type that needs to be handled...

+-----------+-------------+------------------+
| username  | user_avatar | user_avatar_type |
+-----------+-------------+------------------+
| Clockwork | 6680.jpg    | 1                |
+-----------+-------------+------------------+

+----------+------------------+----------------------+
| username | user_avatar      | user_avatar_type     |
+----------+------------------+----------------------+
| Sleipnir | 2_1527436933.gif | avatar.driver.upload |
+----------+------------------+----------------------+

[deleted-user-1]

Looks like our "if type = x" blocks didn't catch his type so the default value from get_type is used in src (the relative url). You can confirm by checking the entry avatar_type entry in the db in the user table for him. I expect you got hit by what I warned in all the previous comments of you ignored and why I suggested to make it all explicit.

[pchote]

+------------------------+----------+
| user_avatar_type       | count(*) |
+------------------------+----------+
|                        |     6582 |
| 0                      |     1489 |
| 1                      |       57 |
| 2                      |        9 |
| 3                      |      103 |
| avatar.driver.gravatar |       57 |
| avatar.driver.remote   |        4 |
| avatar.driver.upload   |      206 |
+------------------------+----------+

define('AVATAR_UPLOAD', 1);
define('AVATAR_REMOTE', 2);
define('AVATAR_GALLERY', 3);

protected $avatar_type_map = array(
    AVATAR_UPLOAD   => 'avatar.driver.upload',
    AVATAR_REMOTE   => 'avatar.driver.remote',
    AVATAR_GALLERY  => 'avatar.driver.local',
);

[pchote]

For now I have just hacked the live copy with 4965addd98c787f59e67d4dfd001bd4cb19b55f8. I expect a proper fix to be merged to master may do things differently.

[dragunoff]

This is probably a legacy mapping of avatar types. I was not aware that it existed. The hotfix seems alright.