Use readonly properties for model classes #217
Conversation
oschwald
commented
Oct 31, 2023
oschwald
commented
- Drop 8.0 support. Test on 8.2.
- Use readonly properties
- Only include set fields in jsonSerlialize output
- Upgrade to phpunit 10
- Use vendor/autoload.php to bootstrap directly
- Do not include name in JSON output
- Make ipAddress and network nullable
This is more similar to the previous version and the output is much more usable, particularly for things like Country.
oschwald
force-pushed
the
greg/readonly-properties
branch
2 times, most recently
from
a3c8dbf to
ee920df
Compare
This makes is more similar to the web-service response.
This is needed for minFraud.
Previously this was not possible as we were using magic methods.
oschwald
force-pushed
the
greg/readonly-properties
branch
from
ee920df to
59aab5d
Compare
phpunit.xml.dist
Outdated
| </testsuite> | ||
| </testsuites> | ||
| <logging/> | ||
| <source> |
There was a problem hiding this comment.
I don't think there is a <source> tag. Did you mean <coverage> ? https://docs.phpunit.de/en/10.0/configuration.html#the-include-element
There was a problem hiding this comment.
Given that phpunit generated it, I assume it is valid, but I'll see if I can clean it up.
There was a problem hiding this comment.
There is an example of its usage in their repo.
See this issue.
phpunit.xml.dist
Outdated
| <logging/> | ||
| <source> | ||
| <include> | ||
| <directory suffix=".php">./src/GeoIp2/</directory> |
There was a problem hiding this comment.
Is the ..../GeoIp2 extra in here ? I can't see the directory within src.
There was a problem hiding this comment.
I think that predates our switch to PSR-4. I'll clean it up.
| new \GeoIp2\Record\Subdivision($sub, $locales) | ||
| ; | ||
| } | ||
|
|
||
| // Not using end as we don't want to modify internal pointer. |
There was a problem hiding this comment.
Would array_key_last help with this ?
There was a problem hiding this comment.
Yes, but that would just turn it to $subdivisions[\array_key_last($subdivisions)]. I am not sure that is a big improvement. I am guessing it would be slower and might make people think it is an associative array as opposed to an indexed one.
phpunit.xml.dist
Outdated
| <directory suffix="Test.php">./tests/GeoIp2/Test/</directory> | ||
| </testsuite> | ||
| </testsuites> | ||
| <logging/> |
There was a problem hiding this comment.
Do we need this tag if it doesn't configure anything ?
There was a problem hiding this comment.
These changes are the result of phpunit --migrate-configuration. I didn't modify the file by hand.
