Skip to content

test phpstan-dba feature branch #1304

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 7 commits into from
Closed

test phpstan-dba feature branch #1304

wants to merge 7 commits into from

Conversation

@staabm
Copy link

@staabm staabm commented May 24, 2022

just testing a bleeding edge phpstan dba feature

@private-packagist
Copy link
Contributor

private-packagist bot commented May 24, 2022 •
edited
Loading

composer.lock

Click to show 192 changes in this composer.lock file

Package changes

Package Operation From To Changes
algolia/algoliasearch-client-php remove 3.2.0 - -
babdev/pagerfanta-bundle remove v3.6.1 - -
bacon/bacon-qr-code remove 2.0.7 - -
beberlei/assert remove v3.3.2 - -
beelab/recaptcha2-bundle remove v2.6.0 - -
cebe/markdown remove 1.2.1 - -
composer/ca-bundle remove 1.3.1 - -
composer/composer remove 2.3.x-dev f1f013e - -
composer/metadata-minifier remove 1.0.0 - -
composer/pcre remove 3.0.0 - -
composer/semver remove 3.3.2 - -
composer/spdx-licenses remove 1.5.6 - -
composer/xdebug-handler remove 3.0.3 - -
dasprid/enum remove 1.0.3 - -
doctrine/annotations remove 1.13.2 - -
doctrine/cache remove 2.1.1 - -
doctrine/collections remove 1.6.8 - -
doctrine/common remove 3.2.2 - -
doctrine/dbal remove 3.3.4 - -
doctrine/deprecations remove v0.5.3 - -
doctrine/doctrine-bundle remove 2.6.0 - -
doctrine/event-manager remove 1.1.1 - -
doctrine/inflector remove 2.0.4 - -
doctrine/instantiator remove 1.4.1 - -
doctrine/lexer remove 1.2.3 - -
doctrine/orm remove 2.11.2 - -
doctrine/persistence remove 2.4.1 - -
doctrine/sql-formatter remove 1.1.2 - -
egulias/email-validator remove 3.1.2 - -
endroid/qr-code remove 4.4.7 - -
ezyang/htmlpurifier remove v4.14.0 - -
friendsofphp/proxy-manager-lts remove v1.0.7 - -
google/recaptcha remove 1.2.4 - -
graze/dog-statsd remove 1.0.0 - -
guzzlehttp/guzzle remove 7.4.2 ⚠️ - -
guzzlehttp/promises remove 1.5.1 - -
guzzlehttp/psr7 remove 2.2.1 - -
justinrainbow/json-schema remove 5.2.11 - -
knplabs/knp-menu remove v3.3.0 - -
knplabs/knp-menu-bundle remove v3.2.0 - -
knpuniversity/oauth2-client-bundle remove v2.10.0 - -
laminas/laminas-code remove 4.5.1 - -
laminas/laminas-diagnostics remove 1.15.0 - -
laminas/laminas-escaper remove 2.10.0 - -
laminas/laminas-feed remove 2.17.0 - -
laminas/laminas-stdlib remove 3.7.1 - -
lcobucci/clock remove 2.1.0 - -
lcobucci/jwt remove 4.1.5 - -
league/oauth2-client remove 2.6.1 - -
league/oauth2-github remove 3.0.0 - -
monolog/monolog remove 2.4.0 - -
nelmio/cors-bundle remove 2.2.0 - -
nelmio/security-bundle remove v2.12.0 - -
pagerfanta/core remove v3.6.1 - -
pagerfanta/doctrine-orm-adapter remove v3.6.1 - -
pagerfanta/twig remove v3.6.1 - -
paragonie/constant_time_encoding remove v2.5.0 - -
phpdocumentor/reflection-common remove 2.2.0 - -
phpdocumentor/reflection-docblock remove 5.3.0 - -
phpdocumentor/type-resolver remove 1.6.1 - -
predis/predis remove v1.1.10 - -
psr/cache remove 2.0.0 - -
psr/container remove 1.1.2 - -
psr/event-dispatcher remove 1.0.0 - -
psr/http-client remove 1.0.1 - -
psr/http-factory remove 1.0.1 - -
psr/http-message remove 1.0.1 - -
psr/link remove 1.1.1 - -
psr/log remove 2.0.0 - -
psr/simple-cache remove 3.0.0 - -
ralouphie/getallheaders remove 3.0.3 - -
react/promise remove v2.9.0 - -
scheb/2fa-backup-code remove v6.0.1 - -
scheb/2fa-bundle remove v6.0.1 - -
scheb/2fa-totp remove v6.0.1 - -
scheb/2fa-trusted-device remove v6.0.1 - -
seld/jsonlint remove 1.8.3 - -
seld/phar-utils remove 1.2.0 - -
seld/signal-handler remove 1.3.0 - -
sensio/framework-extra-bundle remove v6.2.6 - -
snc/redis-bundle remove 3.6.0 - -
spomky-labs/otphp remove v10.0.3 - -
symfony/asset remove v5.4.7 - -
symfony/cache remove v5.4.8 - -
symfony/cache-contracts remove v2.5.1 - -
symfony/config remove v5.4.8 - -
symfony/console remove v5.4.8 - -
symfony/dependency-injection remove v5.4.8 - -
symfony/deprecation-contracts remove v3.0.1 - -
symfony/doctrine-bridge remove v5.4.8 - -
symfony/dotenv remove v5.4.5 - -
symfony/error-handler remove v5.4.8 - -
symfony/event-dispatcher remove v5.4.3 - -
symfony/event-dispatcher-contracts remove v3.0.1 - -
symfony/expression-language remove v5.4.8 - -
symfony/filesystem remove v5.4.7 - -
symfony/finder remove v5.4.8 - -
symfony/flex remove v1.18.6 - -
symfony/form remove v5.4.8 - -
symfony/framework-bundle remove v5.4.8 - -
symfony/http-client remove v5.4.8 - -
symfony/http-client-contracts remove v2.5.1 - -
symfony/http-foundation remove v5.4.8 - -
symfony/http-kernel remove v5.4.8 - -
symfony/intl remove v5.4.8 - -
symfony/lock remove v5.4.7 - -
symfony/mailer remove v5.4.8 - -
symfony/mime remove v5.4.8 - -
symfony/monolog-bridge remove v5.4.3 - -
symfony/monolog-bundle remove v3.7.1 - -
symfony/options-resolver remove v5.4.3 - -
symfony/password-hasher remove v5.4.8 - -
symfony/polyfill-intl-grapheme remove v1.25.0 - -
symfony/polyfill-intl-icu remove v1.25.0 - -
symfony/polyfill-intl-idn remove v1.25.0 - -
symfony/polyfill-intl-normalizer remove v1.25.0 - -
symfony/polyfill-mbstring remove v1.25.0 - -
symfony/polyfill-php72 remove v1.25.0 - -
symfony/polyfill-php73 remove v1.25.0 - -
symfony/polyfill-php80 remove v1.25.0 - -
symfony/polyfill-php81 remove v1.25.0 - -
symfony/process remove v5.4.8 - -
symfony/property-access remove v5.4.8 - -
symfony/property-info remove v5.4.7 - -
symfony/proxy-manager-bridge remove v5.4.6 - -
symfony/routing remove v5.4.8 - -
symfony/security-bundle remove v5.4.8 - -
symfony/security-core remove v5.4.8 - -
symfony/security-csrf remove v5.4.3 - -
symfony/security-guard remove v5.4.3 - -
symfony/security-http remove v5.4.8 - -
symfony/serializer remove v5.4.8 - -
symfony/service-contracts remove v2.5.1 - -
symfony/string remove v5.4.8 - -
symfony/translation remove v5.4.8 - -
symfony/translation-contracts remove v2.5.1 - -
symfony/twig-bridge remove v5.4.8 - -
symfony/twig-bundle remove v5.4.8 - -
symfony/validator remove v5.4.8 - -
symfony/var-dumper remove v5.4.8 - -
symfony/var-exporter remove v5.4.8 - -
symfony/web-link remove v5.4.3 - -
symfony/yaml remove v5.4.3 - -
symfonycasts/verify-email-bundle remove v1.10.0 - -
thecodingmachine/safe remove v2.1.3 - -
twig/extra-bundle remove v3.3.8 - -
twig/string-extra remove v3.3.5 - -
twig/twig remove v3.3.9 - -
ua-parser/uap-php remove v3.9.14 - -
webmozart/assert remove 1.10.0 - -

Dev Package changes

Package Operation From To Changes
doctrine/data-fixtures remove 1.5.2 - -
doctrine/doctrine-fixtures-bundle remove 3.4.1 - -
myclabs/deep-copy remove 1.11.0 - -
nikic/php-parser remove v4.13.2 - -
phar-io/manifest remove 2.0.3 - -
phar-io/version remove 3.2.1 - -
phpspec/prophecy remove v1.15.0 - -
phpstan/phpstan remove 1.5.2 - -
phpstan/phpstan-doctrine remove 1.3.0 - -
phpstan/phpstan-symfony remove 1.1.8 - -
phpunit/php-code-coverage remove 9.2.15 - -
phpunit/php-file-iterator remove 3.0.6 - -
phpunit/php-invoker remove 3.1.1 - -
phpunit/php-text-template remove 2.0.4 - -
phpunit/php-timer remove 5.0.3 - -
phpunit/phpunit remove 9.5.19 - -
sebastian/cli-parser remove 1.0.1 - -
sebastian/code-unit remove 1.0.8 - -
sebastian/code-unit-reverse-lookup remove 2.0.3 - -
sebastian/comparator remove 4.0.6 - -
sebastian/complexity remove 2.0.2 - -
sebastian/diff remove 4.0.4 - -
sebastian/environment remove 5.1.3 - -
sebastian/exporter remove 4.0.4 - -
sebastian/global-state remove 5.0.5 - -
sebastian/lines-of-code remove 1.0.3 - -
sebastian/object-enumerator remove 4.0.4 - -
sebastian/object-reflector remove 2.0.4 - -
sebastian/recursion-context remove 4.0.4 - -
sebastian/resource-operations remove 3.0.3 - -
sebastian/type remove 3.0.0 - -
sebastian/version remove 3.0.2 - -
staabm/phpstan-dba remove 0.2.23 - -
symfony/browser-kit remove v5.4.3 - -
symfony/css-selector remove v5.4.3 - -
symfony/debug-bundle remove v5.4.3 - -
symfony/dom-crawler remove v5.4.6 - -
symfony/maker-bundle remove v1.40.1 - -
symfony/phpunit-bridge remove v5.4.8 - -
symfony/stopwatch remove v5.4.5 - -
symfony/web-profiler-bundle remove v5.4.8 - -
theseer/tokenizer remove 1.2.1 - -

Settings · Docs · Powered by Private Packagist

@private-packagist
Copy link
Contributor

private-packagist bot commented May 24, 2022

The composer.lock diff comment has been updated to reflect new changes in this PR.

@staabm staabm mentioned this pull request May 24, 2022
Merged
@staabm
Copy link
Author

staabm commented May 25, 2022 •
edited
Loading

@chr-hertel since I don't want to annoy Jordi again with my question, I send this question to you - would be great if you could spent a few minutes to answer them.

with this PR I am testing a new phpstan-dba feature, which analyzes sql queries that don't use an index.
as you can see from the 'files changed'-tab, we got a few errors here.

could you have a look at these and tell me whether you would consider those a "false-positive"?
maybe we can figure out, whether/how to read a mysql explain result-set to identify queries which should be optimized by the author.

the current implemenation can be seen in https://github.com/staabm/phpstan-dba/pull/377/files#diff-325c727a8f7834a6b7b42f1b3fbfcaa5c41073e796df7eda0fa5f25a48757eb3R55-R83

@stof
Copy link
Contributor

stof commented May 25, 2022

@staabm looking at the reported issues, they are indeed detecting cases where queries don't use indexes (but then, I'm wondering whether it should always be reported as an error, as having indexes for everything might have drawbacks as it slows down writes, which might not be worth it if a query is used rarely in a CLI command for instance, but that's a separate topic).
The only one that might not be expected is the one reporting a missing index on a derived table (which cannot have an index AFAICT)

@private-packagist
Copy link
Contributor

private-packagist bot commented May 25, 2022

The composer.lock diff comment has been updated to reflect new changes in this PR.

@staabm
Copy link
Author

staabm commented May 25, 2022

@stof thx for the feedback.

ignoring DERIVED-tables seems like a good measure to prevent false-positives -> I will implement just that.

I got another feedback in staabm/phpstan-dba#377 (comment) which made me add a threshold, so we don't report missing indexes/table-scan when tables are small (mysql sometime decides reading the table as is is faster then using an index, when small enough).

@private-packagist
Copy link
Contributor

private-packagist bot commented May 25, 2022

The composer.lock diff comment has been updated to reflect new changes in this PR.

@staabm
Copy link
Author

staabm commented May 25, 2022

just update phpstandba and now we no longer get errors.

it seems the few which were reported previous are related to small tables

@stof
Copy link
Contributor

stof commented May 25, 2022

Well, is the CI running with data in tables or with an empty DB ?

@staabm
Copy link
Author

staabm commented May 25, 2022

I guess its an empty schema currently

see

packagist/.github/workflows/phpstan.yml

Lines 41 to 52 in 6a40c01

- name: "Start MySQL"
run: |
sudo systemctl start mysql.service
echo 'DATABASE_URL="mysql://root:root@127.0.0.1:3306/packagist?serverVersion=8.0"' > .env.dev.local
- name: "Install dependencies"
run: "composer install ${{ env.COMPOSER_FLAGS }}"
- name: "Setup DB"
run: |
bin/console doctrine:database:create -n
bin/console doctrine:schema:create -n

@staabm staabm closed this May 25, 2022
@staabm staabm deleted the dba-perf branch May 25, 2022 18:06
@Seldaek
Copy link
Member

Seldaek commented May 25, 2022

@staabm yeah table size metrics seem to be kinda useless as you're unlikely to have prod data in when running PHPStan. Otherwise great feature even with some amount of false positives IMO.. maybe should be opt-in or at least have an option to opt-out if it is too noisy.

@staabm
Copy link
Author

staabm commented May 25, 2022 •
edited
Loading

thx for the feedback.

maybe should be opt-in or at least have an option to opt-out if it is too noisy.

its an opt-in feature and the min-table size arg can be configured with the RuntimeConfiguration

just added an explicit hint, on how to achieve that scenario

@staabm staabm restored the dba-perf branch May 26, 2022 06:58
@staabm staabm reopened this May 26, 2022
@private-packagist
Copy link
Contributor

private-packagist bot commented May 26, 2022

The composer.lock diff comment has been updated to reflect new changes in this PR.

@Seldaek Seldaek closed this Aug 17, 2022
@staabm staabm deleted the dba-perf branch August 17, 2022 14:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@staabm @stof @Seldaek @clxmstaab