-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Php Inspections (EA Ultimate): minor code tweaks #3200
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kalessil can you check develop
please? I think most of these were already fixed there
@@ -59,7 +59,7 @@ public function connect(array $params, $username = null, $password = null, array | |||
* - passing client_encoding via the 'options' param breaks pgbouncer support | |||
*/ | |||
if (isset($params['charset'])) { | |||
$pdo->query('SET NAMES \''.$params['charset'].'\''); | |||
$pdo->exec('SET NAMES \''.$params['charset'].'\''); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Careful with exec
vs query
- we had issues specifically around switching these in the past, and these changes shouldn't be performed without extreme care here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting, can you share more insight please (so I could document it in the inspection documentation)?. I'll also revert this specific case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't find an issue right now, but we had problems with multiple drivers in switching between query
and exec
for non-SQL stuff such as stored procedures and similar, which do not necessarily use the same underlying protocol for talking between the extension and the SQL server you are connected to.
@morozov can probably say more about that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Ocramius is probably talking about #2752. Also, for the reference, in MySQL listener, we do:
$args->getConnection()->executeUpdate("SET NAMES ".$this->_charset . $collation); |
which internally calls:
dbal/lib/Doctrine/DBAL/Connection.php
Line 1088 in 752f9f5
$result = $this->_conn->exec($query); |
With that in mind, I think the change should be safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for sharing @morozov!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I know about Php Inspections, I was just curious which exact rule from the plugin flagged the issue.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PHP | Php Inspections (EA Extended) | Control Flow | PDO API usage
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, with MySQL the changing from query()
to exec()
can be safe, but must not. PDO maybe is per default not using native prepared statements (https://stackoverflow.com/questions/10113562/pdo-mysql-use-pdoattr-emulate-prepares-or-not). For MSSQL native prepared statements was activated.
As @morozov said, when calling PDO::query()
without parameters it's not the same as PDO::exec()
. I've made a test-script:
$pdo->exec('SET DATEFIRST 1;');
$pdo->query('SET DATEFIRST 1;');
When looking into sys.dm_exec_cached_plans
I find to plans for the two calls:
----------------------------
objtype | text
----------------------------
Adhoc | SET DATEFIRST 1;
Prepared | SET DATEFIRST 1;
Adhoc
queries end up in the standard environment while prepared statements are executed in a separate environment. So if you SET OPTIONS
with prepared statement the next statement will not have access to the new option value.
@kalessil But your inspection gave me the hint what I did wrong, however, it is not generally applicable.
In my case I want to set an option and then reuse it within a prepared statement. So I must set the option with PDO::exec()
and not with PDO::query()
to set it within the standard environment. The next statement can be prepared because the standard environment is copied over to the new environment used for the upcoming prepared statement.
microsoft/msphpsql#385
https://aoeex.com/phile/working-around-scope-identity-not-working-with-pdo
https://docs.microsoft.com/en-us/sql/connect/php/direct-statement-execution-prepared-statement-execution-pdo-sqlsrv-driver?view=sql-server-ver15
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@algorhythm, thanks for the ping. Re-visiting the discussion, we should drop query->exec pattern.
//cc @ea-inspections-team
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the ping @kalessil ! Done, we've dropped the pattern detection.
$tableForeignKeys[$key]['deferrable'] = isset($deferrable[$id]) && 'deferrable' == strtolower($deferrable[$id]) ? true : false; | ||
$tableForeignKeys[$key]['deferred'] = isset($deferred[$id]) && 'deferred' == strtolower($deferred[$id]) ? true : false; | ||
$tableForeignKeys[$key]['deferrable'] = (isset($deferrable[$id]) && 'deferrable' === strtolower($deferrable[$id])); | ||
$tableForeignKeys[$key]['deferred'] = (isset($deferred[$id]) && 'deferred' === strtolower($deferred[$id])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These (...)
are not necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will remove
@Ocramius, checking. |
@Ocramius: nope, not yet fixed in dev. I still can squash changes and target dev with the changes. |
Can you please squash the changes into one commit?
Unless it's a BC break, keep targeting |
@Majkl578 : squashed. |
@@ -65,7 +66,7 @@ public function getSchemaNames() | |||
{ | |||
$rows = $this->_conn->fetchAll("SELECT nspname as schema_name FROM pg_namespace WHERE nspname !~ '^pg_.*' and nspname != 'information_schema'"); | |||
|
|||
return array_map(function ($v) { return $v['schema_name']; }, $rows); | |||
return array_column($rows, 'schema_name'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably not worth reworking but fetchAll(FetchMode::COLUMN)
would be a better approach (also would eliminate the need for the column alias).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me check first: if it not many efforts, I'll do the changes (like the proposal ;))
Cool, can you squash it into one commit once more please? 😀 And don't include "Php Inspections (EA Ultimate):", it doesn't add any value. Thanks. |
@Majkl578 : will squash. Concerning the tool name, that's would be unfair - issues found and fixed by the plugin and it deserves mentioning. |
Tbh, @kalessil is doing these also to show what his tool (out of which he hopes to make a living) is capable of. Leaving in this naming is fair, and it adds a lot of value for him. |
🙄 We've had many of similar PRs in past, we never included "PHPCS", "PHPStan", "some plugin" etc., so this is a bit unfair to the other cases. Mentioning it in PR is imho enough. |
Dunno about you, but I personally write in if a fix was done because of issues detected by tool X, because tool X is indeed worth a mention there. Not sure if I've done it on Doctrine projects, but I would most certainly do it on my stuff anyway. And yes, this is indeed a form of compensation for @kalessil, who patiently followed the issue.
Yes, yes and yes. What does this cost us? Nothing. We're not selling our souls :-) |
Examples: |
Hm: df43437 |
It's worth mentioning, but it's enough to do so in PR, it adds no value in the commit message. |
That is totally different case. PHPStan is part of the project and used as static analysis on CI for each commit/PR. |
It does: readers will be interested in figuring out why these fixes were performed (tool detected them), and they may even be led to finding out about the tool (which tbh is something everyone using this IDE should be using). |
Anyway, build is green and this is going in 👍 |
I'd still make some noise (in the commits, on twitter or on the issue) for @muglug's psalm if he added commits here: the fact that the tool is included or not in the pipeline makes no difference. I'd actually question why we support monsters like Oracle or IBM with our tooling, if you want to bring it to a philosophical level :-P |
Why is obvious without the ad.
Same for the mention in PR itself. (PR is also linked from a commit on GitHub).
This is biased. |
But it's ok to support monsters like Jetbrains or Vimeo? |
Possibly - it's a question of whether these help the ecosystem push forward ( Make some good publicity when you can do it: this is good publicity. |
Sorry, I am not boarding a ship where OSS supports paid products this way. Both Php Inspections (EA Ultimate) (putting stress on Ultimate) and PhpStorm are paid tools. |
By that logic, OracleDB and MsSQL should be gone here. Good examples of this are sponsored audits by companies like ParagonIe or RipsTech - I'd certainly welcome them in exchange of even blog-posts on the official If we start making things hard for contributors that just want a piece of the "inedible visibility cake" then we're just driving useful contributions away: let's not do that. I understand the purism behind your thoughts, but it makes little sense if we're working together for a greater good (and not for ripping off people). We're in symbiosis with many tools, open-source, closed-source, free and paid, and as long as it's for general betterment, giving a push to other projects/people/companies deserving a mention is OK. |
No. By that logic, Oracle DB, MSSQL, MySQL, DB2, SQL Anywhere etc, are supported drivers whereas this is an ad for a paid tool we do not support in DBAL itself (or even use in IDEs since it's paid with no OSS license applicable IIRC).
That is also different situation, and a blog post is also a different way to say thanks.
I don't see anything hard here. Our coding standard is way larger entry barrier. You can freely mention the tool used in the PR itself, in discussion etc., but why should it be in commit message where it doesn't add any value (commit message = what does this commit change?)?
How exactly is an OSS project ripping off anyone by not advertising paid tools with non-free/non-OSS licenses? At least Jetbrains provides free OSS licenses for their tools, Php Inspections (EA Ultimate) does not. I don't think support (as in advertisement in commit messages) for tools that are not free for us belongs here.
open source - ok (PHPCS, PHPStan, Psalm, Travis) |
|
||
return array_map(function ($v) { return $v['schema_name']; }, $rows); | ||
return $statement->fetchAll(FetchMode::COLUMN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this something the inspections found? If so, I'm incredibly impressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, it was suggested in discussion above. :)
#3200 (comment)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, @morozov suggested a better idea for the code fragment we have spotted.
I'm with @Ocramius on this. IMO, exchanging a mention in a single commit for a single commit is fair. Also, I wouldn't mind if somebody submitted a commit like "Fixed issues detected by (Veracode|Checkmarx|HP Fortify)" (if real security issues they can find exist). |
Is this merge commit only available to premium subscribers? |
I think the only problem is when someone starts committing code that can only be generated by a commercial tool. These inspections could be added to Psalm or PHPStan without much difficulty, or really just done with a regex in most cases. If someone were to check in a |
Actually, most of the changes done here should already be covered by our CS (except i.e. swapping trim and strtolower which has no benefit), but not applied in |
@Majkl578 Sure, gonna open a PR in our CS repo with that list :) |
@carusogabriel : please don't. As an author, I against such action and will publically give negative PR (public relation) feedback on such actions. |
Wait what? |
Also we have free OSS licenses: TYPO3, Infection PHP and currently this: https://twitter.com/kalessil/status/1011511612325482496 and ofcourse Php Inspections (EA Extended). |
@Majkl578 : I object on replicating my ideas by 3rd party tooling. If you intended something else, like enabling existing rules, then accept my apologies. |
Gifting few free licenses is not what free for OSS means, there are also free individual licenses for PhpStorm ocassionally. This is how true open source licences look like: |
@Majkl578 you are pretty much picking on an individual versus quite large companies there. Fairly sure @kalessil doesn't currently even have the legal resources to come up with proper licensing for OSS. @kalessil unless ideas are copyrighted, they will be moved into tooling by other parties: that is nothing that can be objected against. |
Sorry, but these are not your ideas, it's part of the PHP language itself. You don't have exclusive rights to write
I don't know if treating OSS requires any additional legal resources, on top of providing paid licenses, isn't it just a matter of here's an invoice for €0 as long as you are provable OSS project?
At least something we agree on here. 👍 |
Agree, part of the language. Nope. I still insist to do your own RnD and see what you say when it getting replicated (or just imagine how this FEELS) ;) I'm pissed off, so marketing is not in the scope right now. @Ocramius : please block discussion if I'm over-doing.
Yes, it does: it also needs to manage licenses (my monthly payment depends on the amount of issued licenses) + I have to estimate support time based on the number of ultimate licenses + once a year to re-issue expired licenses/send invoices. It's me only taking care about the whole project and contributing to OSS project as I get some time free.
Fine, that's why we issuing licenses to trusted OSS projects and not everyone. I'd love to give OSS more licenses, but this thread is a clear indication: no free OSS licenses near future. Mass replication gonna kil the project, which pushed SCA topic a great milestone forward. |
@kalessil I understand your concern - you write code, it provides a service, you see value in that service and so do others, and you've successfully monetised that service. Additionally, the pool of users is not massive, and PHP's userbase is not growing massively, so you're worried that if other tools duplicate some of the functionality it will erode the value of the service you provide. I think the concern is a valid one, but I'm not sure whether the wider community of PHP benefits by having those ways of improving code locked behind a paywall. For example I added dead code detection to (open-source) Psalm based on my experience using Visual Studio (a closed-source product) and improved it after seeing some of the suggestions given by Scrutinizer, another closed-source product. Neither tool invented the concept of identifying dead code, so I didn't feel too terrible about reusing the idea. |
@muglug : it's a little bit different (but partially what you mentioned as well). Currently, the project needs to generate revenue enough for introducing OSS licenses and reducing the prices. So if ideas are getting replicated, we might not reach this point. My task to minimize those risks at all costs. About the community benefits: Php Inspections (EA Extended) maintenance and further development costs are covered by the Ultimate licenses. If the Ultimate is down, Extended will follow shortly. I'm biased and do adopt ideas if only my users implicitly asking to do so (still I decline adopting from commercial products). But we can ask @schmittjoh as well if adopting ideas hurts small businesses (I might be wrong and playing much drama). |
@Majkl578 : I'm cooled off and want to say sorry for un-needed aggression. I should do better. I don't have rights to say individuals, what to do and what don't. I also understand I hope that in near future we'll be able to issue OSS licenses, but only when we can afford it. And I'll keep contributing for sure =) |
I'm not a regular doctrine contributor, but reading the commit message sounded so much as an advertisement as in between of a sport match... of course it helps to finance the match itself... But a match without interruptions for advertisment is way better. |
In my understanding, the major difference between Php Inspections and PHPStan is that the former is an IDE plugin (unlike other CLI-based tools). If I was its author, I wouldn't count on being able to provide the static analysis quality which could compete with existing open source solutions — just based on the number of contributors and adoption of each of them. If the only way to win the competition is negative PR, then the product itself is not really competitive. Instead, the plugin could borrow static analysis ideas from existing tools and focus on providing a superior developer experience as part of the JetBrains platform. |
@goetas: looks like I choose a wrong strategy for advertising. Will re-evaluate. @morozov : yes, my plugins are for IDEs only. We also have different niches with PhpStan and I do recommend to use it additionally to my tools, proof: https://www.youtube.com/watch?v=Vzvfy9m7oes Competition: please note Ondrej was not participating here. Normally project leads (SCA field) are well aware of how to deal with competition and you'll not see any of us following replication strategy. It disrespectful and harmful, instead we are focusing on the strong side of our solutions which are quite unique. |
Release v2.8.0 [![Build Status](https://travis-ci.org/doctrine/dbal.svg?branch=v2.8.0)](https://travis-ci.org/doctrine/dbal) This is a minor release of Doctrine DBAL that aggregates over 30 fixes and improvements developed over the last 3 months. This release includes all changes of the 2.7.x series, as well as feature additions and improvements that couldn’t land in patch releases. **Backwards Compatibility Breaks** This doesn't contain any intentional Backwards Compatibility (BC) breaks. **Dependency Changes** * The dependency on [doctrine/common](https://github.com/doctrine/common) is removed. DBAL now depends on [doctrine/cache](https://github.com/doctrine/cache) and [doctrine/event-manager](https://github.com/doctrine/event-manager) instead. Please see details in the [UPGRADE.md](UPGRADE.md) documentation. **Deprecations** * The usage of binary fields whose length exceeds the maximum field size on a given platform is deprecated. Please use binary fields of a size which fits all target platforms, or use blob explicitly instead. * The usage of DB-generated UUIDs is deprecated. Their format is inconsistent across supported platforms and therefore the feature is not portable. Use a PHP library (e.g. [ramsey/uuid](https://packagist.org/packages/ramsey/uuid)) to generate UUIDs on the application side. **New features** * Initial support of MySQL 8. * Initial support of PostgreSQL 11. * Ability to evaluate arbitrary SQL expressions via `AbstractPlatform::getDummySelectSQL()`. **Improvements and Fixes** * Improved support of binary fields on Oracle and IBM DB2. * Improved SQL Server configuration capabilities via both `sqlsrv` and `pdo_sqlsrv`. * Improved handling of `AUTOINCREMENT`ed primary keys in SQLite. * Integration tests are run against IBM DB2 on Travis CI. * Code coverage is collected for the Oracle platform on continuousphp. Total issues resolved: **33** **Deprecations:** - [3187: Deprecate usage of binary fields whose length exceeds maximum](doctrine#3187) thanks to @morozov - [3188: Deprecated usage of binary fields whose length exceeds the platform maximum](doctrine#3188) thanks to @morozov - [3192: Added more information to the deprecation notice](doctrine#3192) thanks to @morozov - [3212: Deprecated usage of DB-generated UUIDs](doctrine#3212) thanks to @morozov **New Features:** **Bug Fixes:** - [3149: Introduced binary binding type to support binary parameters on Oracle](doctrine#3149) thanks to @morozov - [3178: Fix incorrect exception thrown from SQLAnywhere16Platform](doctrine#3178) thanks to @Majkl578 - [3044: Functional test for allowing dynamic intervals in date sub/add](doctrine#3044) thanks to @AshleyDawson - [3049: Test failures caused by invalid database connection result in fatal error](doctrine#3049) thanks to @Majkl578 **Improvements:** - [3033: Added support for available DSN parameters for the PDOSqlsrv driver](doctrine#3033) thanks to @aashmelev - [3128: Add MySQL 8 reserved keywords](doctrine#3128) thanks to @mlocati - [3143: initialize sql array into platform files](doctrine#3143) thanks to @AlessandroMinoccheri - [3173: Fix composer branch aliases](doctrine#3173) thanks to @Majkl578 - [3157: When building a limit query, zero offset without a limit should be ignored](doctrine#3157) thanks to @morozov - [3109: Allow to specify arbitrary SQL expression in AbstractPlatform::getDummySelectSQL()](doctrine#3109) thanks to @morozov - [3141: allow creating PRIMARY KEY AUTOINCREMENT fields for sqlite (unit tests)](doctrine#3141) thanks to @TimoBakx - [3180: Import simplified version of Common\Util\Debug for var dumping purposes](doctrine#3180) thanks to @Majkl578 **Documentation Improvements:** - [3117: Added badges for the develop branch in README](doctrine#3117) thanks to @morozov - [3125: Upgrading docs](doctrine#3125) thanks to @jwage - [3144: added improvement type into pull request template](doctrine#3144) thanks to @AlessandroMinoccheri **Code Quality Improvements:** - [3025: Added PHPStan, apply changes for level 3](doctrine#3025) thanks to @Majkl578 - [3200: Php Inspections (EA Ultimate): minor code tweaks](doctrine#3200) thanks to @kalessil - [3204: Fix typo in AbstractPlatform](doctrine#3204) thanks to @Majkl578 - [3205: Ignore OCI-* classes in static analysis (no stubs)](doctrine#3205) thanks to @Majkl578 **Continuous Integration Improvements:** - [3102: Use newer PHPUnit to prevent crashes on failures](doctrine#3102) thanks to @Majkl578 - [3112: Removed hard-coded configuration filenames from the test runner](doctrine#3112) thanks to @morozov - [3133: Travis DB2](doctrine#3133) thanks to @Majkl578, @morozov - [3135: AppVeyor tweaks, retry coverage upload on failure](doctrine#3135) thanks to @Majkl578 - [3137: Workaround for the inability to use a post-PHPUnit script on ContinuousPHP](doctrine#3137) thanks to @morozov - [3151: MSSQL DLL 5.2.0 has been released.](doctrine#3151) thanks to @photodude - [3160: Test against Postgres 11](doctrine#3160) thanks to @Majkl578 **Dependencies** - [3193: DBAL 2.8 needs Common 2.9](doctrine#3193) thanks to @Majkl578 - [3176: Eliminate dependency on doctrine/common](doctrine#3176) thanks to @Majkl578 - [3181: Remove dependency on doctrine/common](doctrine#3181) thanks to @Majkl578 # gpg: Signature made Fri Jul 13 06:02:10 2018 # gpg: using RSA key 374EADAF543AE995 # gpg: Can't check signature: public key not found # Conflicts: # .gitignore # README.md # lib/Doctrine/DBAL/Version.php # tests/appveyor/mssql.sql2008r2sp2.sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2012sp1.sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2017.pdo_sqlsrv.appveyor.xml # tests/appveyor/mssql.sql2017.sqlsrv.appveyor.xml # tests/travis/mariadb.mysqli.travis.xml
Minor code tweaks by Php Inspections (EA Ultimate)