Skip to content
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

[bug] Don't skip column options. #3089

Merged
merged 1 commit into from
Apr 6, 2018
Merged

[bug] Don't skip column options. #3089

merged 1 commit into from
Apr 6, 2018

Conversation

and1truong
Copy link
Contributor

No description provided.

@and1truong and1truong changed the title Don't skip column options. [bug] Don't skip column options. Apr 5, 2018
@and1truong
Copy link
Contributor Author

Very unstable Travis );

@and1truong
Copy link
Contributor Author

@Majkl578 can you review?

Copy link
Contributor

@Majkl578 Majkl578 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just few nitpicks. :)

Introduced in #2846.

@@ -125,14 +125,14 @@ public function setOptions(array $options)
foreach ($options as $name => $value) {
$method = "set".$name;
if ( ! method_exists($this, $method)) {
// next major: throw an exception
// next major: throw an exception.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems unrelated.

* @group legacy
* @expectedDeprecation The "unknown_option" column option is not supported, setting it is deprecated and will cause an error in Doctrine 3.0
*/
public function testOptionsShouldNotBeIgnored()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please add void return type?

public function testOptionsShouldNotBeIgnored()
{
$col1 = new Column('bar', Type::getType(Type::INTEGER), ['unknown_option' => 'bar', 'notnull' => true]);
$this->assertTrue($col1->getNotnull());
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use static assertion calls using self::.

@and1truong
Copy link
Contributor Author

All fixed per your suggestion. Thanks for fast response @Majkl578.

Copy link
Member

@greg0ire greg0ire left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh right, looks like I forgot this was in a foreach, sorry for this :(

@Majkl578 Majkl578 merged commit 9e757bf into doctrine:master Apr 6, 2018
@Majkl578
Copy link
Contributor

Majkl578 commented Apr 6, 2018

@andytruong Thanks!

Majkl578 added a commit to Majkl578/doctrine-dbal that referenced this pull request Apr 6, 2018
@Majkl578
Copy link
Contributor

Majkl578 commented Apr 7, 2018

Backported into 2.7 @ 2f64799

@and1truong and1truong deleted the dont-skip-column-options branch April 7, 2018 02:17
@Majkl578 Majkl578 self-assigned this Apr 7, 2018
@Majkl578 Majkl578 added the BC Fix label Apr 7, 2018
rgrellmann added a commit to Rossmann-IT/dbal that referenced this pull request Apr 13, 2018
This release fixes unintentional BC breaks:

1. It was impossible to use deprecated fetch modes with PDO-based drivers.
2. An unsupported option passed to the `Column` object prevented subsequent options from being applied.
3. Date interval values stored prior to upgrade to `v2.7.0` were reported as invalid.

Total issues resolved: **10**

**Backwards Compatibility Fixes:**

- [3082: Custom PDO fetch modes and 2.7.0](doctrine#3082) thanks to @corphi
- [3088: Fix doctrine#3082: Add BC for PDO-only fetch modes](doctrine#3088) thanks to @corphi
- [3089: Don't skip column options.](doctrine#3089) thanks to @andytruong
- [3093: When updating to version v2.7 type DateInterval throws errors](doctrine#3093) thanks to @fnagel
- [3097: Fix compatibility for pre-2.7 DateIntervalType format](doctrine#3097) thanks to @Majkl578

**Documentation Improvements:**

- [3083: Document the correct way of configuring a MariaDB database with serverVersion](doctrine#3083) thanks to @tristanbes
- [3084: README: Add 2.7, drop 2.5](doctrine#3084) thanks to @Majkl578

**Continuous Integration Improvements:**

- [3085: Tests: remove implicit verbose flag](doctrine#3085) thanks to @Majkl578
- [3090: Add symfony tests listener](doctrine#3090) thanks to @greg0ire
- [3095: CI: Add missing listener for MariaDB @ mysqli](doctrine#3095) thanks to @Majkl578

# gpg: directory `/n/.gnupg' created
# gpg: new configuration file `/n/.gnupg/gpg.conf' created
# gpg: WARNING: options in `/n/.gnupg/gpg.conf' are not yet active during this run
# gpg: DBG: locking for `/n/.gnupg/pubring.gpg.lock' done via O_EXCL
# gpg: keyring `/n/.gnupg/pubring.gpg' created
# gpg: Signature made Sun Apr  8 07:24:49 2018     using RSA key ID 543AE995
# gpg: Can't check signature: public key not found

# Conflicts:
#	.gitignore
#	lib/Doctrine/DBAL/Driver/OCI8/Driver.php
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants