Ensure column options map to an existing method by greg0ire · Pull Request #2846 · doctrine/dbal

greg0ire · 2017-09-06T18:27:05Z

No description provided.

greg0ire · 2017-09-13T13:52:39Z

lib/Doctrine/DBAL/Schema/Column.php

-            if (method_exists($this, $method)) {
-                $this->$method($value);
+            if (!method_exists($this, $method)) {
+                throw InvalidArgumentException::fromUnsupportedOption($name);


Might be considered a BC-break, tell me if you want me to trigger deprecation errors instead.

I notice that it does not seem to be used anywhere in the code, but maybe you're not against it?

Yes, this could indeed be considered unnecessary BC break in 2.x for no real benefit... And I'm not even sure deprecation notice is worth it. Maybe we should just retarget this to develop/3.0?

How about retargetting only this part, and merging the rest in master, since it's BC?

for no real benefit...

A smooth upgrade path for people misusing this method is a good benefit IMHO. And the number of misuses I found in doctrine/dbal alone suggests this might be useful to many, don't you think.

Agreed, I meant there is "no real benefit" in throwing an exception in 2.x due possible BC concerns.
Anyway, I'm not sure what is the correct approach regarging deprecations. I think the silent variant (what i.e. Symfony does) @triger_error('blahblah', E_USER_DEPRECATED) would be just fine. We will eventually need to deprecate multiple things anyway at latest in last 2.x before 3.0 comes (especially in ORM).

Great, I'll go with that then!

It's funny to see old options in the tests, that had no use at all. I like the stricter handling with that exception, but I agree on BC and the deprecation trigger.

To test it properly, I will need to make simple-phpunit work though :(
Right now I get:

PHP Fatal error: Class 'Doctrine\Tests\DbalTestCase' not found in /home/greg/dev/dbal/tests/Doctrine/Tests/DBAL/Cache/QueryCacheProfileTest.php on line 9 Fatal error: Class 'Doctrine\Tests\DbalTestCase' not found in /home/greg/dev/dbal/tests/Doctrine/Tests/DBAL/Cache/QueryCacheProfileTest.php on line 9

greg0ire · 2017-09-19T06:32:14Z

ping

Majkl578 · 2017-09-20T02:49:28Z

lib/Doctrine/DBAL/Schema/Column.php

            $method = "set".$name;
-            if (method_exists($this, $method)) {
-                $this->$method($value);
+            if (!method_exists($this, $method)) {


There should be a space around !.

Majkl578 · 2017-09-20T02:51:27Z

lib/Doctrine/DBAL/Schema/Column.php

-            if (method_exists($this, $method)) {
-                $this->$method($value);
+            if (!method_exists($this, $method)) {
+                throw InvalidArgumentException::fromUnsupportedOption($name);


Yes, this could indeed be considered unnecessary BC break in 2.x for no real benefit... And I'm not even sure deprecation notice is worth it. Maybe we should just retarget this to develop/3.0?

SenseException · 2017-09-20T20:51:12Z

lib/Doctrine/DBAL/Exception/InvalidArgumentException.php

        return new self('Empty criteria was used, expected non-empty criteria');
    }
+
+    public static function fromUnsupportedOption(string $name): self


Spaces between return types.

greg0ire · 2017-09-20T22:17:47Z

.travis.yml

  - travis_retry composer -n install

-script: ./vendor/bin/phpunit --configuration tests/travis/$DB.travis.xml
+script: SYMFONY_DEPRECATIONS_HELPER=weak_vendors ./vendor/bin/phpunit --configuration tests/travis/$DB.travis.xml


Not using the simple-phpunit binary because of a crash I don't understand (seems to have to do with autoloading).
The "weak_vendors" will make the tests suite fail if I forget to remove any call to setOption with an unknown option, unless it's in a test marked with the "@legacy" annotation. The test suite won't fail if a vendor suddenly adds a deprecation (that's why it's called weak vendors, it's lenient with deprecations from vendors).

More details here. And yes, I'm the author of that feature :P

I pushed something new. Turns out you don't have to use simple-phpunit to test deprecations properly.

greg0ire · 2017-09-20T22:18:22Z

composer.json

        "symfony/console": "2.*||^3.0",
-        "doctrine/coding-standard": "^1.0",
-        "squizlabs/php_codesniffer": "^3.0"
+        "symfony/phpunit-bridge": "^3.3"


Adding this so that we get a report of deprecations at the end of the test suite. It's not necessary to use the simple-phpunit binary to get it.

Is everyone on board with this?

What's the purpose of reporting the usage of deprecated code in a test run? As far as I understand, the code is deprecated for the outside usage but it still has to be tested until removed entirely.

@morozov you are right in that in needs to be tested. Such tests are supposed to be marked with a dedicated annotation @group legacy, so that it does not make the build fail. But if somewhere in the lib, we call the deprecated API ourselves, we must be aware of it, and change that. If we, don't users will get a deprecation and won't be able to do anything about it. That's what this is about.

@greg0ire thanks for the explanation. Makes sense.

greg0ire · 2017-09-20T22:19:01Z

composer.json

    "bin": ["bin/doctrine-dbal"],
+    "config": {
+        "sort-packages": true
+    },


Sorted packages in a separate commit, please review commit by commit.

greg0ire · 2017-09-20T22:19:29Z

lib/Doctrine/DBAL/Exception/InvalidArgumentException.php

+    public static function fromUnsupportedOption(string $name) : self
+    {
+        return new self(sprintf('The "%s" option is not supported', $name));
+    }


Unused at the moment, should be used in 3.0.

Should I remove it or should I strive to make the diff of the 3.0 PR minimal?

I'd remove it since it's not used anywhere.

greg0ire · 2017-09-20T22:21:05Z

tests/Doctrine/Tests/DBAL/Schema/ColumnTest.php

        self::assertEquals($expected, $this->createColumn()->toArray());
    }

+    /**


I was not able to use the simple-phpunit binary, which means I can't test the deprecation message as described here: https://symfony.com/doc/current/components/phpunit_bridge.html#write-assertions-about-deprecations

UPDATE: no it does not mean that, and yes I was able to test it properly.

greg0ire · 2017-09-21T11:54:55Z

lib/Doctrine/DBAL/Schema/Column.php

-                $this->$method($value);
+            if ( ! method_exists($this, $method)) {
+                // next major: use InvalidArgumentException::fromUnsupportedOption()
+                @trigger_error(sprintf(


Scrutinizer does not like the silencing

It seems like Scrutinizer allows silencing in an if block, but that's no option for this case. I haven't worked much with the Scrutinizer config, but maybe there's a way to ignore it just for trigger_error.

Maybe this link would help: https://scrutinizer-ci.com/docs/reviews/filtering_issues_and_false_positives ?

@lcobucci maybe you can help us here?

Could anyone please point me to the article explaining why this silencing operator is used in Symfony and we want it here? As far as I understand, the built-in PHP error handler will not display such errors. A user-defined handler, if it respects error_reporting(), it will ignore such errors as well. Does it mean users will have to use some specialized error handler to catch these errors?

@morozov here is the handler: https://github.com/symfony/symfony/blob/master/src/Symfony/Bridge/PhpUnit/DeprecationErrorHandler.php IIRC most implementations ignore silenced errors, this one does not. I think a PSR is coming regarding all this.

We want it here not only for Symfony users, but for us: we want to know if we use a deprecated path somewhere else in the lib

greg0ire · 2017-09-21T17:12:06Z

phpunit.xml.dist


    <listeners>
        <listener class="Doctrine\Tests\DbalPerformanceTestListener"/>
+        <listener class="Symfony\Bridge\PhpUnit\SymfonyTestsListener" />


This is how you get to test deprecations with annotations

Not a fan of this, but hey, it's dev-only, so I can live with it.

greg0ire · 2017-09-21T17:40:58Z

tests/Doctrine/Tests/DBAL/Schema/ColumnTest.php


+    /**
+     * @group legacy
+     * @expectedDeprecation The "unknown_option" option is not supported, setting it is deprecated and will cause an exception in 3.0


The build passed from red to green after adding this line... not sure why: symfony/symfony#21896

greg0ire · 2017-09-28T15:37:31Z

I remove the weak_vendors mode as asked by @Ocramius , please review again.

greg0ire · 2017-12-07T16:27:32Z

@morozov thanks for making me dig deeper, I understand it better too now :)

Majkl578 · 2017-12-07T16:32:37Z

lib/Doctrine/DBAL/Schema/Column.php


 namespace Doctrine\DBAL\Schema;

+use Doctrine\DBAL\Exception\InvalidArgumentException;


This use is unused.

Majkl578 · 2017-12-07T16:34:28Z

lib/Doctrine/DBAL/Schema/Column.php

+                // next major: throw an exception
+                @trigger_error(sprintf(
+                    'The "%s" option is not supported,'.
+                    ' setting it is deprecated and will cause an exception in 3.0',


Probably should not mention exception directly (implementation detail), just error.

People do not need to know that this will be implemented as an exception.

Majkl578 · 2017-12-08T02:04:20Z

@Ocramius Are you on board with using @trigger_error(..., E_USER_DEPRECATED) pattern for deprecations?

Ocramius · 2017-12-08T09:08:51Z

@Majkl578 yes, this is something I discussed in person with @nicolas-grekas, and the approach is solid.

Ocramius · 2017-12-08T09:12:14Z

🚢

Ocramius · 2017-12-08T09:12:56Z

@greg0ire can I ask you to rename the PR title to something more clear to future readers?.

greg0ire · 2017-12-08T09:32:15Z

@Ocramius sure!

morozov · 2018-03-31T18:58:43Z

tests/Doctrine/Tests/DBAL/Schema/ColumnTest.php

+     * @group legacy
+     * @expectedDeprecation The "unknown_option" option is not supported, setting it is deprecated and will cause an error in 3.0
+     */
+    public function testSettingUnknownOptionIsStillSupported() : void


@greg0ire, in some cases, this test is reported as risky:

There was 1 risky test:

Doctrine\Tests\DBAL\Schema\ColumnTest::testSettingUnknownOptionIsStillSupported
This test did not perform any assertions
...
Legacy deprecation notices (1)

Is it expected?

Older versions of Symfony's phpunit bridge do not count expected deprecation as assertion (and thus mark the test risky).

I would advice to always run all test jobs with the latest version of the bridge

@wouterj could you suggest the proper composer.json constraints? Currently, we have:

dbal/composer.json

Line 24 in 46069b6

"symfony/phpunit-bridge": "^3.4.5|^4.0.5"

Dropping the left part of the pipe is completely fine IMO.

@greg0ire on the build in question, this constraint was resolved to v4.0.6.

Ah sorry I misread this as 4.0.6 fixes the issue… latest is 4.0.7

Can we somehow isolate the issue and report it to upstream?

I'm trying to reproduce it as we speak but I'm afraid it only ever happens on windows. Should I create a PR with zero tests but the faulty one and let appveyor show the bug?

It's fine if it helps to isolate the issue.

Ok then. See symfony/symfony#26809

and1truong · 2018-04-06T03:29:22Z

lib/Doctrine/DBAL/Schema/Column.php

+                    $name
+                ), E_USER_DEPRECATED);
+
+                return $this;


What is it @andytruong ?

…ine API.

greg0ire force-pushed the validate_options branch 5 times, most recently from 5c2747d to 4cfec3a Compare September 11, 2017 18:47

greg0ire commented Sep 13, 2017

View reviewed changes

Majkl578 previously requested changes Sep 20, 2017

View reviewed changes

Majkl578 added Improvement Schema Schema Definition Deprecation labels Sep 20, 2017

SenseException reviewed Sep 20, 2017

View reviewed changes

greg0ire force-pushed the validate_options branch 3 times, most recently from 65a4dcd to b388d42 Compare September 20, 2017 22:14

greg0ire commented Sep 20, 2017

View reviewed changes

greg0ire commented Sep 21, 2017

View reviewed changes

greg0ire force-pushed the validate_options branch from b388d42 to cecbbbd Compare September 21, 2017 17:06

greg0ire commented Sep 21, 2017

View reviewed changes

greg0ire force-pushed the validate_options branch from cecbbbd to 2b38d9f Compare September 21, 2017 17:32

greg0ire commented Sep 21, 2017

View reviewed changes

greg0ire force-pushed the validate_options branch 3 times, most recently from 6753d83 to 87c56a3 Compare September 28, 2017 15:08

Majkl578 reviewed Dec 7, 2017

View reviewed changes

Grégoire Paris added 2 commits December 7, 2017 17:34

Remove unused use statement

9213360

Use abstract wording

e8209e0

People do not need to know that this will be implemented as an exception.

greg0ire force-pushed the validate_options branch from db6853a to e8209e0 Compare December 7, 2017 19:07

Majkl578 mentioned this pull request Dec 8, 2017

Deprecation of EntityManager copy method doctrine/orm#6803

Merged

Majkl578 approved these changes Dec 8, 2017

View reviewed changes

Majkl578 mentioned this pull request Dec 8, 2017

[QueryBuilder] setParameter : trim the input key when comparing it to… doctrine/orm#5996

Closed

Ocramius approved these changes Dec 8, 2017

View reviewed changes

Ocramius self-assigned this Dec 8, 2017

Ocramius added this to the 2.7.0 milestone Dec 8, 2017

Ocramius merged commit cb08c4d into doctrine:master Dec 8, 2017

greg0ire deleted the validate_options branch December 8, 2017 09:32

greg0ire changed the title ~~Validate options~~ Ensure column options map to an existing method Dec 8, 2017

Majkl578 mentioned this pull request Feb 16, 2018

PHPUnit 7 #3009

Merged

morozov reviewed Mar 31, 2018

View reviewed changes

Majkl578 mentioned this pull request Mar 31, 2018

Always use phpunit bridge 4 #3076

Closed

and1truong reviewed Apr 6, 2018

View reviewed changes

morozov mentioned this pull request Sep 7, 2018

Extend SQLServerPlatform Tests to inherit previous version's test functions #3270

Closed

alcaeus mentioned this pull request Aug 1, 2019

The "nullable" column option is not supported, setting it is deprecated; No alternative doctrine/annotations#275

Closed

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 6, 2021

[doctrineGH-2846] Convert unknown Column options deprecation to Doctr…

f9c73a6

…ine API.

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 7, 2021

[doctrineGH-2846] Convert unknown Column options deprecation to Doctr…

c7225fe

…ine API.

beberlei added a commit to beberlei/dbal that referenced this pull request Mar 22, 2021

[doctrineGH-2846] Convert unknown Column options deprecation to Doctr…

c98bccb

…ine API.

github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022


		namespace Doctrine\DBAL\Schema;

		use Doctrine\DBAL\Exception\InvalidArgumentException;

Uh oh!

Conversation

greg0ire commented Sep 6, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire commented Sep 19, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morozov Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Sep 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

greg0ire Sep 20, 2017 •

edited

Loading

greg0ire Sep 20, 2017 •

edited

Loading

morozov Dec 6, 2017 •

edited

Loading

greg0ire Dec 6, 2017 •

edited

Loading

greg0ire Sep 20, 2017 •

edited

Loading