Feature/improve implementation

[nyamsprod]

Improve the PHP codebase to make it easier to debug.

so instead of doing

$avatarId = "Binx Bond";
$multiavatar = new Multiavatar($avatarId, false, ['part' => '11', 'theme' => 'B']);
echo($multiavatar->svgCode);

Now the code behave like this

$avatarId = "Binx Bond";
$multiavatar = new Multiavatar();
echo $multiavatar($avatarId, false, ['part' => '11', 'theme' => 'B']);

The svgCode public property was no-where to be found.
The changes makes instantiating the Multiavatar class only once to generate as many avatar as you want.

Depending on your comment I may improve the changes.

What I did not do, but migth be added in a Futur PR: ...

• Move the code under a namespace;
• Moving the $sansEnv into the $ver arguments of the Multiavatar::__invoke method
• Adding tests;

PS: Adding tests should be IMHO left apart as you have 2 separate implementations the best way would be to have a test suite independent of the implementation language 😉 .

[shadowtime2000]

can't review php, i barely know anything about it

[shadowtime2000]

did you base this off of what the current file was or did you just make it?

[nyamsprod]

No. This is based on how to easily autoformat the package files on IDE. It is used as a baseline so that PHPStorm or Vscode or any IDE can easily format the file on saving or on editing. Most PHP libraries comes with those to have a unify coding practice/style.

[shadowtime2000]

I know what .editorconfig is, I was just asking how you came up with the values for the configuration.

[nyamsprod]

The value are the on I use ... the default is set for PHP according to PHP standard coding style from PSR-1

The other also stem form standard practices in PHP world

[shadowtime2000]

Ah, makes sense.

[giekaton]

@nyamsprod that's cool. Thanks! There is definitely room for improvement, as this was my first attempt at making a proper PHP package.

@shadowtime2000 I was planning to check your PR tomorrow, so I will check both now.

Thanks for contributions!

[giekaton]

Impressive code, and many changes, I'm still reading through them, and trying to understand everything.

A few initial comments:

Because of the Multiavatar naming guidelines, it should be written either Multiavatar, or multiavatar, but never multiAvatar or MultiAvatar. Other variables being in camelCase is good, similarly as in the JS code.

Why do you suggest moving $sansEnv boolean into the $ver array? It's a different type of a parameter, serves a different purpose, and it's easier to use it if you only need an avatar without the env part, e.g.: $multiavatar('x', true, null); VS. $multiavatar('x', ["sansEnv" => true]);. The second one would also require additional checking for the missing $ver properties. Also, for consistency, it follows the same logic as in the JS script, so if there is a good reason to change it here, then it should probably be changed in the JS script also.

The tests from the index.php file are for the PHP implementation only, as the syntax is different from JS, so there were additional nuances to test for. To test the visual representation of avatars, while designing them, I was using the html files from the /svg folder.

What benefits would namespacing provide in Multiavatar's case?

I see some new coding concepts introduced. What's the minimum PHP version required to run your code? I'm planning to use this PHP code in a WordPress plugin, and some WordPress sites are still running on old versions of PHP.

[nyamsprod]

@giekaton thanks for the kind words

Because of the Multiavatar naming guidelines, it should be written either Multiavatar, or multiavatar, but never multiAvatar or MultiAvatar.

As far as I know I did not change the naming guideline for the class but I can double check if that's the case I'll correct it

Other variables being in camelCase is good, similarly as in the JS code.

This is done in accordance with PHP guidelines from PSR-1 and PSR-12

Why do you suggest moving $sansEnv boolean into the $ver array? It's a different type of a parameter, serves a different purpose,

From a maintainer point of view you are right the parameters are differents. But from a consumer of your library point of view it's just another options so for that consumer the signature would easier to remember in the form of

$multiavatar = new Multiavatar();
$options = [
    'sansEnv' => true,  
    'skin' => [
        'part' => '05',  
        'theme' => 'C',
    ],
    'gender' => 'female', // easier to expand if needed
];
$svg = $multiavatar('avatar', $options);

As long as the $options are well defined and filtered you should have no overhead and the variable can easily be extend to add more feature without modifying the overall API. You could even convert the array structure into a PHP object if needed. Bottom line you reduce cognitive information on how to use the library.

Also, for consistency, it follows the same logic as in the JS script, so if there is a good reason to change it here, then it should probably be changed in the JS script also.

Again, for consistency I did not make the change as I think you are the one to make that call and I agree that having a similar API in both language gives a better DX.

What benefits would namespacing provide in Multiavatar's case?

Namespacing in PHP was introduced to avoid class name collision between libraries. By adding a Multiavatar namespace you declare to the world that anything inside that namespace belongs to you and any collision with someone else code is not your responsability 👍

What's the minimum PHP version required to run your code? I'm planning to use this PHP code in a WordPress plugin, and some WordPress sites are still running on old versions of PHP.

AFAIK Wordpress requires PHP7.3 now so yes the current code will work in PHP7.3 . I did not check but the code should work unchanged in PHP7.1. Before, more checks to perform and I would avoid trying to work on lesser version of PHP just for a question of maintenance. But again that's my point of view.

The tests from the index.php file are for the PHP implementation only, as the syntax is different from JS, so there were additional nuances to test for. To test the visual representation of avatars, while designing them, I was using the html files from the /svg folder.

If you want I can try to convert those tests using PHPUnit but that will go in a different PR.

[giekaton]

Regarding the camelCase in $multiAvatar, you have used it here, that's why I mentioned it. So let's keep the $multiavatar in lower case, and all other vars in camelCase. I will include this info later in contributing guidelines.

For the $options, you have a good point that if we add gender or race options, it would be better to keep them in a single array. Another good point is that these extra additions wouldn't introduce breaking changes for the API.

So if you can implement $options, that would be great, only instead of the skin name, let's use ver, to have similar naming as in JS code for now, and later I will think how better to unify both codebases.

I have asked about namespacing because it seems that the Multiavatar name is unique in PHP environments, so there would be no collisions anyway. But I guess it's still better to have namespacing, as this would be a future-proof solution.

Some WordPress or other PHP-based sites still run on PHP 5. I have just tried to change my PHP version to 5.6.40 and to run both PHP scripts. Mine was producing the expected results, yours had errors because of the new PHP features used:

Warning: Unsupported declare 'strict_types' in Multiavatar.php on line 3

Parse error: syntax error, unexpected 'const' (T_CONST), expecting variable (T_VARIABLE) in Multiavatar.php on line 15

I will do some more research to check if it makes sense to support PHP 5 these days.

[giekaton]

And for tests, it would be great to present all different 48 base versions, similarly as in this html file. In order to catch bugs, the representation of such a test should be visual.

I can add all versions to the index.php file (similarly like I did in html), but maybe you can suggest a better option.

I have just found out that the ["part" => "08", "theme" => "C"] in the PHP script is missing its mouth part, so I'm going to debug this, but also need to check all 48 base versions. That's what I meant when talking about PHP specific nuances, because in the JS script everything works well.

Also, just saw that the part value must be passed as a string, because an integer starting with zero (e.g. 08) doesn't work.

[giekaton]

This was not a PHP-specific issue, but simply the theme colors were mixed between eyes and mouth for the 08 character. Now I have fixed this.

For the future, it would be great to have all 48 base characters presented visually because not all bugs have programmatic errors. If an equal length color array is mixed, or a double semicolon appears in a color string, PHP doesn't show an error in such cases.

So as mentioned above, I can make a simple index.php file that would visually present all 48 base versions, unless you can suggest a better solution. The meaning of such a test is that if all 48 base versions are good, then it means that all 12 billion are also good.

[nyamsprod]

@giekaton my PR did fix the issue between integer and string mix in PHP 😉. For the rest I'll check later and sync my PR with the changes you made on master and refactor it to use the $options variable. ... responding from my phone

[nyamsprod]

@giekaton so I've updated the PR according to your comments here's what I've added:

• I've added the $options parameters
• the theme parameter is now case-insensitive
• the part parameter can be an integer, a string, a string with a leading 0
• I've added a test suite using PHPUnit (the code is therefore tested on PHP7.2+)
• I've added Github actions to make sure the tests are being run in different PHP versions
• I've added a demo folder containing the "visual" tests as you requested
• I've updated the themes according to the modification on the master
• I've put the code under the Multiavatar namespace

The only remaining bug is one that was already there. Once I've implemented the visual tests the following reference svg contains wrong information:

<?php

use Multiavatar/Multiavatar;

$multiavatar = new Multiavatar();
$svg = $multiavatar('a', ['ver' => ['part' => 15, 'theme' => 'b']]);

The generated avatar is different from the one generated via Javascript (the avatar hair color are different). I have pin point the issue in the code that replace colors but I failed to find the solution. seems some replacement are wrongly done. Maybe you should investigate the use of preg_replace_callback instead of doing it in two steps 🤔 .

Either way the infrastructure is there to help you fix the issue.

[nyamsprod]

@giekaton I was able to fix the issue with the following tests

<?php

use Multiavatar/Multiavatar;

$multiavatar = new Multiavatar();

$svg = $multiavatar('a', ['ver' => ['part' => 15, 'theme' => 'b']]);
$svg = $multiavatar('a', ['ver' => ['part' => 9, 'theme' => 'b']]);
$svg = $multiavatar('a', ['ver' => ['part' => 9, 'theme' => 'c']]);

I rewrote the svgElements colorizing method to instead use preg_replace_callback and I had to change the order of the theme color as the rendered SVG in the Javascript script had the color in a different order.

So the PR is all good now 😉

[giekaton]

Awesome! I'm going to review and test everything and then will reply with a more detailed response.

I'm still thinking that it's a good idea to support PHP 5. According to the latest data, more than 30% of ALL websites are still running on PHP 5. We would lose this group of users if PHP 5 is not supported.

If this was a library for a modern PHP developer, that would be a different thing, as developers often update their stack, but users who use avatars are not always tech-savvy (e.g. just running a WP site with plugins), so there is a big chance that we will lose many of them.

So in general, it would be great to support PHP 5. Maybe the new breaking PHP concepts can be rewritten in an old-school way, or somehow made backward-compatible?

[shadowtime2000]

I don't know much about PHP, but I am pretty sure there are tools or something that can transpile newer PHP to older PHP. I don't remember the name though.

[nyamsprod]

@shadowtime2000 there's not much to transpile ... downgrading the code can be done quite easily without removing the current settings. There's only one function that requires polyfilling everything else should work as is by just removing typehinting 😢 and strict type 😢 then again there are two way to adress this
you can downgrade it all ...

or release two versions one for PHP5.6 until PHP7.1 and this version ...

I'll let you decide I can create another PR which is PHP5.6 compliant if needed

[giekaton]

Thanks for this additional info. I will check everything, and think about what's best, and then let you know.

Having two versions is also an option, as I can then use the PHP 5 version for WP plugin. How would you suggest to structure the repository in such a case? It would be best to keep both versions in the same repo and in the same branch, just different instructions in the README, one for the modern PHP developer, and one for the PHP 5.

[giekaton]

Also, the earliest version having a significant share is PHP 5.2, so maybe target this one, or at least 5.3?

[nyamsprod]

@giekaton targeting PHP5.3 is really a hard downgrade which will indeed require a lot more work, you will loose more. IMHO you should target PHP5.6 because even Wordpress says they don't support anything less. At some point you need to fix a trade off and leave some users behind for their own good or ask for a paid supports for those who still want to run an insecure, outdated version of PHP at least that my POV.

Requirements
To run WordPress we recommend your host supports the following:
PHP version 7.3+
MySQL version 5.6+ or MariaDB version 10.1+
HTTPS support
We recommend Apache and NGINX as the most robust servers with the most features for running WordPress, but any server >that supports PHP and MySQL will do.
Note: WordPress also works in legacy environments with PHP 5.6.20+ and MySQL 5.0+, but these versions have reached official end-of-life and as such may expose your site to security vulnerabilities.

[giekaton]

Yes, WP has dropped support for PHP 5, but many users are still running on old WP (or other PHP-based frameworks).

I will think about this more... Let me know if you have an idea for the repository structure.

I think that the PHP 5 version can be a static build, and if new Multiavatar features will be added in the future, they may not neccessary be included in the PHP 5 version.

[giekaton]

I have tested my code on PHP 5.3 and PHP 5.5.

On 5.3 it doesn't work because PHP 5.3 doesn't support the [ ] array syntax. As you have mentioned, that would be a significant rewrite since a huge part of the code is using the array syntax. So no need to support PHP 5.3.

It works on PHP 5.5, and it should also work on PHP 5.4, but I haven't got a chance to test it on 5.4 yet.

For the structure, maybe we can have your code downgraded to PHP 5 version (MultiavatarPHP5.php), located in the src folder among other scripts.

I'm thinking about using the PHP 5 version for the WordPress plugin, because being a part of the plugin, it provides only a single function (to return SVG code by ID). The end-user will not see or use the actual generator's code, but only interact through the WordPress UI. So in such a case, it makes sense to use the version with the widest possible support.

I'm also following your commits and testing them locally. Love the code, no additional comments yet.