AbstractOptions isset issue, #7286 #7287
Conversation
…ns that doesn't have the variable, an exception is thrown.
|
Perhaps you can split up and create a separate PR for each issue? I see a fix for |
| @@ -60,6 +60,7 @@ public function render(ElementInterface $element) | |||
| $hiddenAttributes = array( | |||
| 'name' => $attributes['name'], | |||
| 'value' => $element->getUncheckedValue(), | |||
| 'disabled' => isset($attributes['disabled']) ? $attributes['disabled'] : false, | |||
|
@rms230 : Please read our contributing guide, particularly the section linked on branching for patches. The pull request here combines two separate features, which should be evaluated separately; this is due to doing both on your master branch. Also make sure you run php-cs-fixer (which is installed by default when you run You can create a new branch from your master branch, revert the unwanted commits (the stuff around the form components), and create a new pull request from there. The changes do look good; I just want to make sure they come in cleanly. Thanks! |
The test checks that the exception was not thrown, this checks that future implementations do not revert changes were made. Checking for an exception thrown would cause the tests to fail. How would I go about doing this with the standards?
|
@rms230 If you want this included in 2.4.0, you have until tomorrow, 19 March 2015, to resubmit as a discrete patch against develop as outlined in my previous comment. |
| $element->setUseHiddenElement(true); | ||
| $element->setAttribute('disabled', true); | ||
| $markup = $this->helper->__invoke($element); | ||
| $this->assertRegexp('#type="checkbox".*?(disabled="disabled")#', $markup); |
There was a problem hiding this comment.
Just tested this and this test already passes against current master.
|
@weierophinney I created a separate PR for the checkbox issue and also updated the unittest so it fails against current master and passes with the provided fix from @rms230 See #7341. |
|
Closed in favor of #7341 |
|
@weierophinney Please see PR #7342 for the second part of this PR about the |
When calling isset() on an object variable that is a class extending AbstractOptions an exception is thrown.
An example useage of this is found in the EventManagerAwareTrait where the trait checks a class variable isset, if the class that is using the EventManagerAwareTrait extends AbstractOptions an exception is thrown.
This pull request fixes AbstractOptions by checking that the method exists and that the value is not equal to null, thus stopping the exception being thrown.
This pull request also fixes issue #7286