Skip to content
This repository has been archived by the owner on Jan 8, 2020. It is now read-only.

Fix: Remove unused parameter and class property #6914

Conversation

localheinz
Copy link
Member

Injecting $options has no effect and class property $options is never used.

@@ -54,8 +54,6 @@ public function __construct($connection, Statement $statementPrototype = null, R
$connection = new Connection($connection);
}

$options = array_intersect_key(array_merge($this->options, $options), $this->options);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm thinking it's supposed to be

$this->options = array_intersect_key(array_merge($this->options, $options), $this->options);

Copy link
Member Author

Choose a reason for hiding this comment

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

@macnibblet

Shall I update?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes

@localheinz localheinz force-pushed the bugfix/unused-local-variable branch from d53b936 to 4009c06 Compare November 23, 2014 10:43
@localheinz localheinz changed the title Fix: Unused local variable Fix: Value should probably be assigned to property rather than local variable Nov 23, 2014
@@ -54,7 +54,7 @@ public function __construct($connection, Statement $statementPrototype = null, R
$connection = new Connection($connection);
}

$options = array_intersect_key(array_merge($this->options, $options), $this->options);
$this->options = array_intersect_key(array_merge($this->options, $options), $this->options);
Copy link
Member

Choose a reason for hiding this comment

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

Still looks like it's fixing something. What was broken before this change, @macnibblet?

Copy link
Contributor

Choose a reason for hiding this comment

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

After reviewing the code closely my recommendation would be to remove $options from the constructor and $this->options from the class since they are not actually used.

Copy link
Member Author

Choose a reason for hiding this comment

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

@macnibblet

Since injecting $options hasn't had any effect: 👍

@localheinz localheinz force-pushed the bugfix/unused-local-variable branch from 4009c06 to 8a44be4 Compare November 23, 2014 15:58
@localheinz localheinz changed the title Fix: Value should probably be assigned to property rather than local variable Fix: Remove unused parameter and class property Nov 23, 2014
@localheinz localheinz force-pushed the bugfix/unused-local-variable branch from 8a44be4 to 561f927 Compare November 23, 2014 15:58

);

/**
* @param array|Connection|\oci8 $connection
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 change oci8 to Oci8

Copy link
Member Author

Choose a reason for hiding this comment

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

@macnibblet

If that's what it should be, this needs a fix in more than one file.

Copy link
Contributor

Choose a reason for hiding this comment

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

No nevermind, that was a misstake.

Ocramius added a commit that referenced this pull request Nov 23, 2014
@Ocramius Ocramius closed this in 46d9fad Nov 23, 2014
@Ocramius
Copy link
Member

Merged \o/

master: 46d9fad
develop: 86668de

@Ocramius Ocramius self-assigned this Nov 23, 2014
@Ocramius Ocramius added this to the 2.3.4 milestone Nov 23, 2014
@localheinz localheinz deleted the bugfix/unused-local-variable branch November 24, 2014 12:43
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.

3 participants