search

phpstan / phpstan-doctrine

Doctrine extensions for PHPStan
MIT License
598 stars 97 forks source link

Allow to test hydration mode in platform DB. #587

Closed VincentLanglet closed 5 months ago

VincentLanglet commented 5 months ago

WDYT @janedbal ? is it a good way to introduce test about different hydration mode ?

janedbal commented 5 months ago

I was thinking about just using all hydratation modes that should be equal in performDriverTest. Just fetch it multiple ways and assert those match.

VincentLanglet commented 5 months ago

I was thinking about just using all hydratation modes that should be equal in performDriverTest. Just fetch it multiple ways and assert those match.

But hydration mode doesn't always give the same result like bigInt which is a numeric-string with hydrate_object but an int with hydrate_scalar. Also, depending on having hydrate_scalar, hydrate_single_scalar, hydrate_scalar_colum, the result is an array of array, just a scalar or an array of scalar.

janedbal commented 5 months ago

Since 95% of the dataset in platform test uses DQL expressions, those should behave equally in all hydratation modes (I think, didnt check). Then there are few cases with entity field fetches (+ TypedExpressions) which would differ. So maybe we can keep your MR and assert those assumptions:

VincentLanglet commented 5 months ago

Since 95% of the dataset in platform test uses DQL expressions, those should behave equally in all hydratation modes (I think, didnt check).

For instance, SELECT o.bigIntColumn from ... will return

So for the same query,

The second point need extra logic, I currently don't have in the PR. But the first point...

Then there are few cases with entity field fetches (+ TypedExpressions) which would differ. So maybe we can keep your MR and assert those assumptions:

  • no mode given (null) = expects ALL modes to return the same
  • specific mode given = expects the given mode to return the given expected value

Yes, this could be a nice solution.

So I updated the PR to use null by default to go in this direction. https://github.com/phpstan/phpstan-doctrine/pull/587/commits/b3b2f9915316868bfc7683a0ba0663d2772e795e

But it still require to add extra logic to check ALL modes return the same. And so far this dev does not seems to be an easy one to me, specially because I didn't succeed running the docker locally...

janedbal commented 5 months ago

Specially because I didn't succeed running the docker locally...

How is that, which part of the readme is wrong?

VincentLanglet commented 5 months ago

Specially because I didn't succeed running the docker locally...

How is that, which part of the readme is wrong?

When I run

docker-compose -f tests/Platform/docker/docker-compose.yml up -d

I get

#0 69.09 E: Package 'msodbcsql17' has no installation candidate
------
failed to solve: process "/bin/sh -c apt update      && apt install -y gnupg2     && apt install -y unixodbc-dev unixodbc     && curl https://packages.microsoft.com/keys/microsoft.asc | apt-key add -     && curl https://packages.microsoft.com/config/debian/11/prod.list > /etc/apt/sources.list.d/mssql-release.list     && apt update     && ACCEPT_EULA=Y apt install -y msodbcsql17     && pecl install sqlsrv-5.11.1     && pecl install pdo_sqlsrv-5.11.1     && docker-php-ext-enable sqlsrv pdo_sqlsrv" did not complete successfully: exit code: 100

and didn't found time to debug this ATM

janedbal commented 5 months ago

I checked this a bit and I think it should be better to create separate regular test for it. We dont really need all drivers to be tested for hydratation modes imo. Also, you cannot test e.g. HYDRATE_SIMPLEOBJECT in platform one.

janedbal commented 5 months ago

Here is a draft of how it might look like: https://github.com/phpstan/phpstan-doctrine/pull/590

ondrejmirtes commented 5 months ago

Superseded by https://github.com/phpstan/phpstan-doctrine/pull/590