Yubico / php-u2flib-server

(OBSOLETE) U2F library in PHP
https://developers.yubico.com/php-u2flib-server/
BSD 2-Clause "Simplified" License
289 stars 68 forks source link

Close issue 45: add PSR-4 auto-loading #49

Closed djfurman closed 2 years ago

djfurman commented 8 years ago

Created a Yubico namespace for PSR-4 auto-loading. Updated Composer.json to utilize the Yubico namespace and map the src/u2flib_server directory. Updated U2F.php file to incorporate PSR-4 auto-loading. Updated composer.json development dependencies to include PHPUnit for testing purposes. Updated PHPUnit.xml to include all existing tests located in the tests directory by default. Ensured that all PHPUnit tests return green.

This closes issue 45.

djfurman commented 8 years ago

To pass I need to have Travis CI run composer dump-autoload before running the PHPUnit tests on the ./tests directory.

As I have little experience with Travis, I'll begin researching how to accomplish this and provide a revised pull request.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 98.773% when pulling d128217eef9f33e88fdb3bb6cc1f7a8d0401ed21 on djfurman:master into e27822e1ab42b6df9ed5fda8cd7bd1377ae02d64 on Yubico:master.

djfurman commented 8 years ago

I've resolved the Travis-CI issue by adding an install step to direct composer to create the auto-loaders. Please consider this pull request to close issue 45 for the library.

coveralls commented 8 years ago

Coverage Status

Coverage increased (+0.1%) to 98.773% when pulling d128217eef9f33e88fdb3bb6cc1f7a8d0401ed21 on djfurman:master into e27822e1ab42b6df9ed5fda8cd7bd1377ae02d64 on Yubico:master.

klali commented 8 years ago

Moving this to a Yubico namespace seems good to me.

I'm not to keen on making a hard dependency on composer though, is there any reason why the tests can't keep doing require on the U2F.php file? And composer can be an option for those that want to use it.

Spomky commented 8 years ago

To avoid the require_once in each test file, you have to specify to PHPUnit a bootstrap file (i.e. the autoloader computed by composer).

<phpunit bootstrap="vendor/autoload.php" colors="true">...

Then, you can remove all require_once(__DIR__ . '/../vendor/autoload.php');

djfurman commented 8 years ago

Cool @Spomky, I didn't know you could do that with PHPUnit!

Updated to reflect this.

djfurman commented 8 years ago

@klali, for the tests we don't have to, but I don't have a good way to test the autoloader with PHPUnit to ensure that PSR-4 is autoloading appropriately.

Would it be preferred to have two test files, one to include the direct reference and the other autoloading?

paragonie-scott commented 8 years ago

Can this be merged any time soon?

I want to contribute to this library but I don't want to create merge conflicts and a lot of the pain preventing me from contributing is that my IDE will complain about not conforming to PSR-4.

djfurman commented 8 years ago

@klali , How can I help resolve this and help with the other open issues?

klali commented 8 years ago

First off: sorry for dropping the ball on this.

I'd like to merge this without the hard dep on composer. I like that the project works fine out of the box without it (or am I missing something and being wrong here?)

djfurman commented 8 years ago

To add PSR-4, we'd have to have some kind of autoloader.

That being said, we may be able to write a conditional to see if an autoloader is available and if not, then go back to direct file requirements instead of autoloading. My concern would be that this will result in less clean code after addressing some of the other open issues such as class separation. That is trivial with PSR-4, without, it really gets to be a pain.

paragonie-scott commented 8 years ago

I'd like to merge this without the hard dep on composer. I like that the project works fine out of the box without it (or am I missing something and being wrong here?)

Most PHP developers are going to be using Composer anyway. For the ones that aren't, you can do what defuse/php-encryption does and distribute a .phar instead.

StormTide commented 8 years ago

PSR-4 is a FIG standard, doesnt require a hard-dep on composer. Example autoloaders are found at https://github.com/php-fig/fig-standards/blob/master/accepted/PSR-4-autoloader-examples.md

klali commented 8 years ago

So parts of my reluctance with composer comes from #2 Is the current best practice for php now to use composer?

StormTide commented 8 years ago

@klali The composer MITM has been resolved, so its no longer a mitm by reference. As for best-practice? No. That would be PSR-4 (which is entirely separate from composer).

djfurman commented 8 years ago

Thanks, @StormTide .

@klali , to clarify: The code changes themselves are only implementing the PSR-4 convention. The PHPUnit test is using composer to activate that, just like different versions of PHP.

dav-is commented 7 years ago

Update on this?

mdeboer commented 7 years ago

Would like to know as well, PSR-4 and a proper Yubico namespace would be nice.

stephanvierkant commented 6 years ago

Any update?

Fixes #43 and #45. (please including this text in your PR, so Github will close the issues automatically after merging this PR.)

djfurman commented 3 years ago

Anything I can do to resolve any issues with this PR?

emlun commented 2 years ago

Thank you for your contributions, and sorry this never went anywhere. Since the U2F API is now obsolete, we're archiving this repository and ceasing maintenance.