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

Proper PHP support #74

Merged

Conversation

Radiergummi
Copy link
Contributor

@Radiergummi Radiergummi commented Dec 9, 2020

This PR adds a simple wrapper for PHP applications that basically replicates the JavaScript exports as namespaced, autoloaded functions, that can be easily imported.

The problem

Using this library in PHP currently requires to load the JSON files from the correct filesystem path inside the vendor directory (Laravel example):

private function getCountries(): array
{
    $raw = file_get_contents(base_path(
        'vendor/annexare/country-list/dist/minimal/countries.en.min.json'
    ));

    return json_decode(
        $raw,
        true,
        512,
        JSON_THROW_ON_ERROR
    );
}

This means I have a hard dependency on the folder structure staying exactly the same across releases, and I don't even trust myself enough to maintain something like this correctly all the time.

Proposed solution

This PR adds a wrapper file, providing all exports from the JavaScript entry file as functions that can be imported thanks to an autoloader entry in the composer.json. To prevent unnecessary disk access, all functions use a super-simple loader that caches the file content for optimal performance.

It can be used like this:

require __DIR__ . '/vendor/autoload.php';

// Function imports
use function Annexare\Countries\continents;
$continents = continents();

// Fully-qualified, namespaced function calls
$countries = \Annexare\Countries\countries();

This makes it easy to work with the library, and simultaneously shifts responsibility for the proper file names from consumers to the library.

A note on the flag emoji helpers:
I started recreating the emoji functions in PHP, but that requires more time and testing than I'm willing to bring up currently. You could either leave them out entirely (they are JS-exclusive at the moment, anyway) or finish the implementations. I'm not really convinced they add much value for PHP apps, but who am I to decide this.

@Radiergummi Radiergummi changed the title Proper php support Proper PHP support Dec 9, 2020
@dmythro
Copy link
Member

dmythro commented Dec 9, 2020

Hi, thanks for this!
I'll check this out. Not working with PHP for a long time, so have to do a live test for this :)

Or, maybe, there is a way to add couple unit tests to check the exports for PHP?

@dmythro dmythro self-requested a review December 9, 2020 16:48
@dmythro dmythro added the update label Dec 9, 2020
@Radiergummi
Copy link
Contributor Author

Yeah, of course! Adding tests using PHPUnit is definitely possible, but that means a bunch of config files, build scripts and test files. I wanted to keep it simple to see if you're interested in the idea at all, but I can add that too :)

@dmythro
Copy link
Member

dmythro commented Dec 27, 2020

@Radiergummi I don't have PHP in projects for a while, let's be honest here :) But there is a composer package, which I was using on own projects too, back in time, with older PHP versions. So honestly I'm not aware if changes like this will still work there, or it's a breaking change since some versions etc.

If someone who uses PHP could support this and we had some basic unit tests to run/verify on PRs — I'd of course would be glad to go for it. Also I'd add those tests to CI then too, to be sure the package is alright (need to check the proper way of testing it, maybe for couple different PHP versions even as far as PHP 8.0 was released).

But I suppose it's a one-time work, so please ping me if we'll go for it.

P.S. Current PR already looks good to me, but can't check it quickly while on vacation.

@Radiergummi
Copy link
Contributor Author

@dmythro Sorry, Christmas preparations got me. I'll take care of it the next days 🙂

Will update the PR soon, happy holidays!

@dmythro
Copy link
Member

dmythro commented Dec 27, 2020

@dmythro Sorry, Christmas preparations got me. I'll take care of it the next days 🙂

Will update the PR soon, happy holidays!

Sounds good!

By the way, does auto-complete work with your solution? Or maybe something else should be added for that, like PhpDoc?

@dmythro
Copy link
Member

dmythro commented Dec 27, 2020

By the way, about Emoji. There already are Emoji (both Emoji and code) for each country in the countries.emoji.min.json data. Not sure if it's used by someone on backend side, but you could just use that and search by the country code in data. Shouldn't use much more memory on load.

@Radiergummi
Copy link
Contributor Author

So I've just added a few basic tests that make sure the data loaded by the helpers is exactly the same as loading the actual JSON files and parsing them with PHP. This should make sure the helpers do exactly what they're supposed to.
To run the tests in Travis would require adding a matrix configuration I guess, but I've not done that yet.


By the way, does auto-complete work with your solution? Or maybe something else should be added for that, like PhpDoc?

It does, kind of. IDEs will auto-complete the functions as you type them, and know they return an array. But there's not really a good way to type hint array structure in PHP yet.

@dmythro
Copy link
Member

dmythro commented Jan 4, 2021

Thanks, sounds good! I'll check running tests on CI then, and we're good to go.

@dmythro dmythro changed the base branch from master to php-support January 6, 2021 17:37
Copy link
Member

@dmythro dmythro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merging this to a local branch to finish on CI.

@dmythro dmythro merged commit e56f38c into annexare:php-support Jan 6, 2021
@dmythro dmythro mentioned this pull request Jan 6, 2021
@Radiergummi
Copy link
Contributor Author

Nice. Thank you :)

@dmythro
Copy link
Member

dmythro commented Jan 6, 2021

No, thank you! :)
Sorry for a delay, a bit out of schedule now but will release this soon I think.

dmythro added a commit that referenced this pull request Jan 8, 2021
* Proper PHP support (#74)

* Create index.php

* Added autoloader entry

* Removed Emoji functions due to PHP complexity issues

* Updated code style to match library

* Added PHPUnit

* Added basic test implementation

* ci: check php-actions/phpunit@v2

* ci: remove nodejs 10

* ci: PHP config updates

* ci: nodejs 10 lives till about October '21 :)

* v2.6.0

Co-authored-by: Moritz Friedrich <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants