Closed dakujem closed 5 years ago
Cool, thanks! I'll look at it tomorrow. Only one note - it breaks some tests, can you fix them? Just change
$reflection = Abc::getReflection()
to
$reflection = Abc::getReflection();
$reflection->getEntityProperties();
Thanks!
I see. Now that the props are lazy loaded, the constructor does not trigger the errors. I will update the tests.
Anyway, what is your configuration to run th tests? I did not find any info on running tests, or contributing in general.
I use vendor/bin/tester -p php -c tests/php-unix.ini tests/
(on Ubuntu 16.04 with PHP 7.2).
1) composer install
2) edit tests/php-unix.ini
(if needed)
3) vendor/bin/tester -p php -c tests/php-unix.ini tests/
I updated the failing test. I also added another optimization (I forgot to include it yesterday) that enables on-demand getter/setter initialization. The performance impact of it is not so remarkable, but removes some unncessary method name parsings.
By the way, the tests fail using PHP 7.2.4:
-- FAILED: Entity.emptyQuery.phpt
Exited with error code 255 (expected 0)
E_WARNING: count(): Parameter must be an array or an object that implements Countable
in tests\LeanMapper\Entity.emptyQuery.phpt(38)
-- FAILED: Entity.assign.1.phpt
Exited with error code 255 (expected 0)
E_DEPRECATED: The each() function is deprecated. This message will be suppressed on further calls
in tests\LeanMapper\Entity.assign.1.phpt(77)
in tests\LeanMapper\Entity.assign.1.phpt(77) each()
vendor/bin/tester -p php -c tests/php-unix.ini tests/
does not really work, because at least sqlite is needed. I'm running the tests in my local environment using tester v2 an the switch -C
(using my local cli configuration). I suggest using this configuration, contributing will be easier for general public.
I'm also using this snippet in composer.json
:
"scripts": {
"test":"tester tests -C"
}
then it is very easy to just type composer test
and have the tests running.
I updated the failing test.
Please also update Entity.hasMany.inversed.phpt
(see https://travis-ci.org/Tharos/LeanMapper/builds/425134127)
on-demand getter/setter initialization
You read my mind :)
...tests fail using PHP 7.2.4
Have you latest version? It was fixed in 62a7363395fbebab27a080231d9f3d465ee436e3.
because at least sqlite is needed.
Yeah, the tests currently require json
,pdo
, sqlite3
andtokenizer
extensions (on my configuration). They are listed in php-unix.ini
.
Sorry, guys. I did not realize that I had forked the repo before 🤣 I was thinking I was working on the latest version, because I clicked the fork button on the day I created the PR... Anyway, I synced the repo and all the tests are passing now. Even those 7.2 issues disappeared, as you mentioned.
@janpecha I believe the PR is now ready to be merged.
@dakujem Can you squash commits into single commit with message (for example):
EntityReflection: improved performance
Added on-demand property parsing and getter/setter initialization.
Thanks!
Hey, I thought GitHub offers this feature when merging. If not, let me know and I will squash.
Yes, Github offers this feature, but from my point of view is better if author of PR prepare it because final commit is tested on CI once again before merging.
Merged, thanks!
I will do that next time.
The problem:
The solution:
The result:
The test that shows the problem can be seen here: https://github.com/dakujem/LeanMapper/blob/props-parsing-hell/tests/LeanMapper/EntityReflection.parsing.phpt
The tests for the fixed version can be found here: https://github.com/dakujem/LeanMapper/blob/props-parsing-test/tests/LeanMapper/EntityReflection.parsing.phpt
I did not include the test intentionally, as it requires a static hook in the EntityReflection class or changing the visibility of methods in the class (private->protected). I am open to discussion.