[Parameter] Deprecate names starting or ending with a colon #6880

fancyweb · 2017-12-12T14:09:31Z

I wasn't able to make a test as I don't know how to test a deprecation message without the @expectedDeprecation annotation of the Symfony PHPUnit Bridge ^^

Ocramius · 2017-12-12T14:12:13Z

Requires a test case

fancyweb · 2017-12-12T14:15:33Z

@Ocramius As I said in my comment I need help for this test.

coudenysj · 2017-12-12T14:36:18Z

@fancyweb Just construct the class with a faulty string, and watch what happens.

By default, PHPUnit will install an error handler that converts the following errors to exceptions: https://phpunit.de/manual/current/en/appendixes.configuration.html

fancyweb · 2017-12-12T14:39:35Z

I tried that and it was just marked as a risky test since there was no assertion. No exception was thrown.

coudenysj · 2017-12-12T14:42:44Z

Probably because you use the @ operator, I'll try to checkout your branch and have a look.

Majkl578 · 2017-12-12T15:31:56Z

Please wait until #6869 is merged (after 2.6 is released), it will add the infrastructure for testing deprecations.

lcobucci · 2019-11-15T13:56:18Z

Just missing the test so I'll keep on 2.7.0 and we can easily test it with things added in #7901

lcobucci · 2019-11-16T01:29:19Z

@fancyweb thanks, I've updated things to add the test. I'm just struggling on explaining the value of this deprecation in UPGRADING.md. I'll move this 2.8.0 so we have enough time to discuss this 👍

SenseException · 2019-11-19T22:46:00Z

Thanks for adding the tests.

I'm just struggling on explaining the value of this deprecation in UPGRADING.md

Maybe describe it as a clear information about how a parameter has to look like and being less ambiguous?

lcobucci · 2019-11-20T07:03:26Z

@SenseException focusing on the ambiguity is a good idea. I guess what made me hesitate too was doing the check using regex. We could do the trim and then compare with the original value, sending the deprecation notice when they are different.

greg0ire · 2019-11-20T07:51:38Z

lib/Doctrine/ORM/Query/Parameter.php

@@ -67,6 +69,10 @@ class Parameter
     */
    public function __construct($name, $value, $type = null)
    {
+        if (preg_match('/^:|:$/', $name)) {
+            @trigger_error('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0', E_USER_DEPRECATED);


Let's add the package name and use some linebreaks:

Suggested change

@trigger_error('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0', E_USER_DEPRECATED);

@trigger_error(

'Starting or ending a parameter name with ":" is deprecated since Doctrine ORM 2.7 and will cause an error in 3.0',

E_USER_DEPRECATED

);

greg0ire · 2019-11-20T07:52:32Z

tests/Doctrine/Tests/ORM/Query/ParameterTest.php

+     */
+    public function deprecationMustBeTriggeredWhenUsingColonInParameterNames() : void
+    {
+        $this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0');


Suggested change

$this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0');

$this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since Doctrine ORM 2.7 and will cause an error in 3.0');

It should be 2.8 too :)

greg0ire · 2019-11-20T07:52:43Z

tests/Doctrine/Tests/ORM/Query/ParameterTest.php

+        $this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0');
+        new Parameter(':user_name', 'Testing');
+
+        $this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0');


Suggested change

$this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0');

$this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since Doctrine ORM 2.7 and will cause an error in 3.0');

greg0ire · 2019-11-20T07:52:53Z

tests/Doctrine/Tests/ORM/Query/ParameterTest.php

+        $this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0');
+        new Parameter('user_name:', 'Testing');
+
+        $this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0');


Suggested change

$this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0');

$this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since Doctrine ORM 2.7 and will cause an error in 3.0');

SenseException · 2019-11-24T01:01:00Z

lib/Doctrine/ORM/Query/Parameter.php

@@ -67,6 +69,10 @@ class Parameter
     */
    public function __construct($name, $value, $type = null)
    {
+        if (preg_match('/^:|:$/', $name)) {


As @lcobucci wrote:

We could do the trim and then compare with the original value, sending the deprecation notice when they are different.

derrabus · 2022-05-11T10:05:10Z

Can we pick this up for 2.13?

Ocramius added Deprecation Improvement Missing Tests labels Dec 12, 2017

Ocramius assigned fancyweb Dec 12, 2017

Majkl578 added this to the 2.7.0 milestone Dec 17, 2017

Majkl578 changed the base branch from master to 2.7 December 22, 2017 20:27

Majkl578 force-pushed the 2.7 branch from aeef8fa to 1951401 Compare August 8, 2018 18:03

Majkl578 force-pushed the 2.7 branch from 4e71572 to 01cff97 Compare September 23, 2018 05:14

Majkl578 force-pushed the 2.7 branch from 01cff97 to 62fb493 Compare November 11, 2018 22:32

lcobucci force-pushed the 2.7 branch from 62fb493 to c036c0b Compare November 21, 2018 00:41

lcobucci force-pushed the 2.7 branch from 9e73672 to 40f2406 Compare September 23, 2019 09:29

lcobucci force-pushed the 2.7 branch from 40f2406 to d9c30e3 Compare October 23, 2019 13:59

[Parameter] Deprecate names starting or ending with a colon

1712fc1

lcobucci force-pushed the deprecate-colon-in-parameter-name branch from 1deb37b to 1712fc1 Compare November 16, 2019 01:27

lcobucci removed the Missing Tests label Nov 16, 2019

lcobucci modified the milestones: 2.7.0, 2.8.0 Nov 16, 2019

greg0ire reviewed Nov 20, 2019

View reviewed changes

SenseException requested changes Nov 24, 2019

View reviewed changes

lcobucci changed the base branch from 2.7 to 2.8.x November 29, 2019 10:31

beberlei modified the milestones: 2.8.0, 2.8.1, 2.8.2 Dec 4, 2020

beberlei removed this from the 2.8.2 milestone Dec 13, 2020

derrabus changed the base branch from 2.8.x to 2.13.x May 11, 2022 10:01

derrabus mentioned this pull request May 11, 2022

[Parameter] Prevent names from starting or ending with a colon #6881

Closed

fancyweb closed this Oct 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Parameter] Deprecate names starting or ending with a colon #6880

[Parameter] Deprecate names starting or ending with a colon #6880

fancyweb commented Dec 12, 2017 •

edited

Ocramius commented Dec 12, 2017

fancyweb commented Dec 12, 2017

coudenysj commented Dec 12, 2017

fancyweb commented Dec 12, 2017

coudenysj commented Dec 12, 2017

Majkl578 commented Dec 12, 2017

lcobucci commented Nov 15, 2019

lcobucci commented Nov 16, 2019

SenseException commented Nov 19, 2019

lcobucci commented Nov 20, 2019 •

edited

greg0ire Nov 20, 2019

greg0ire Nov 20, 2019

lcobucci Nov 20, 2019

greg0ire Nov 20, 2019

greg0ire Nov 20, 2019

SenseException Nov 24, 2019

derrabus commented May 11, 2022

	$this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since 2.7 and will cause an error in 3.0');
	$this->expectDeprecationMessage('Starting or ending a parameter name with ":" is deprecated since Doctrine ORM 2.7 and will cause an error in 3.0');

[Parameter] Deprecate names starting or ending with a colon #6880

[Parameter] Deprecate names starting or ending with a colon #6880

Conversation

fancyweb commented Dec 12, 2017 • edited

Ocramius commented Dec 12, 2017

fancyweb commented Dec 12, 2017

coudenysj commented Dec 12, 2017

fancyweb commented Dec 12, 2017

coudenysj commented Dec 12, 2017

Majkl578 commented Dec 12, 2017

lcobucci commented Nov 15, 2019

lcobucci commented Nov 16, 2019

SenseException commented Nov 19, 2019

lcobucci commented Nov 20, 2019 • edited

greg0ire Nov 20, 2019

Choose a reason for hiding this comment

greg0ire Nov 20, 2019

Choose a reason for hiding this comment

lcobucci Nov 20, 2019

Choose a reason for hiding this comment

greg0ire Nov 20, 2019

Choose a reason for hiding this comment

greg0ire Nov 20, 2019

Choose a reason for hiding this comment

SenseException Nov 24, 2019

Choose a reason for hiding this comment

derrabus commented May 11, 2022

fancyweb commented Dec 12, 2017 •

edited

lcobucci commented Nov 20, 2019 •

edited