mapkyca / known2fa

Two Factor Authentication for Known users
3 stars 2 forks source link

Vendor folder to link specific commit and repo #8

Open Lewiscowles1986 opened 4 years ago

Lewiscowles1986 commented 4 years ago

Looking at this it's very cool.

The only thing that confused me was not documenting the repo and commit for the QR-Generator-PHP dependency.

It looks like Terence Eden has a GitHub repo. They work or did work for the UK Government and have interacted, not just forked the repo, so it might be a good source (I'm assuming the original was harder to track?)

https://github.com/edent/QR-Generator-PHP

Would you be open to a PR for this?

mapkyca commented 4 years ago

The vendor directory in this plugin was created by hand and the plugin itself was created long before Known moved over to composer. While I made the plugin itself composer installable, I didn't go back and tidy up that directory like I should.

Yes, the QR generator is Terence Eden's one, it's simple and works great in native PHP (plugin used to shell out to a console command which meant it'd only work on linux, and definitely not on shared hosts).

Might be worth swinging back and installing dependencies "properly" via composer, but I think this would require us adopting Terence's code and making it composer installable.

I'm waffling!

Anyway, definitely open to a PR to tidy this up a bit!

Lewiscowles1986 commented 4 years ago

Cool, I've contacted Terence on the repo to see if I can get or have him add to packagist. It really will (I believe) help people to understand systems better.

I'll launch a PR once I can test it.

If Terence cannot or will not publish to packagist, or suggests an alternative, this may morph into using a different library, rather than maintaining a fork; especially given the shell out part of the above description, for which I'd raise a separate issue, get that merged unblocking this.

Thanks for :eyes: It had not escaped my attention, it's just more work tracking and interacting with dependencies vendored in this way.