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

Fix for #3515, functional extension & tests #1456

Merged
merged 1 commit into from Aug 30, 2013

Conversation

ravage84
Copy link
Member

https://cakephp.lighthouseapp.com/projects/42648-cakephp/tickets/3515

I used (string) because it is faster than strval() and there are no real benefits in this case.
Also enabled assign(), append() and prepend() to take any values convertible to string.

Removed try/catch as discussed with ADmad.
Changed the three exception expecting tests to check for PHPUnit_Framework_Error now.

@Phally
Copy link
Contributor

Phally commented Jul 27, 2013

👍

}
if (!isset($this->_blocks[$name])) {
$this->_blocks[$name] = '';
}
if ($mode === ViewBlock::PREPEND) {
$this->_blocks[$name] = $value . $this->_blocks[$name];
$this->_blocks[$name] = (string)$value . $this->_blocks[$name];
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this but the concatenation will implicitly cast to string so the explicit casting isn't needed.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for confirming 😄

@jippi
Copy link
Contributor

jippi commented Jul 27, 2013

👍 to docblocks + test
👎 to code changes

@markstory
Copy link
Member

Being able to use objects that implement __toString is a nice improvement. If you remove the explicit string casts I think this is good to go.

@ravage84
Copy link
Member Author

@markstory explicit string casts are now removed, objects with __toString are supported now. I assume I should add a note in the migration guide about the new support of concat() & assign() for to string convertible values as well?

if (!is_string($value)) {
throw new CakeException(__d('cake_dev', 'Blocks can only contain strings.'));
if (is_array($value) ||
is_object($value) && !method_exists($value, '_toString')) {
Copy link
Member

Choose a reason for hiding this comment

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

This will do is_array() and is_object() check for all values when in most cases it's going to a string. I am just guessing but maybe doing something like this might be more efficient / faster.

try {
    $this->_blocks[$name] = (string)$value;
} catch (Exception $e) {
    throw new CakeException(__d('cake_dev', 'Blocks can only process strings or values convertible to string.'));
}

Copy link
Member

Choose a reason for hiding this comment

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

Btw the magic method is __toString, not _toString, that's why your test case fails 😄

Copy link
Member

Choose a reason for hiding this comment

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

Err typecasting an array to string gives Array so my suggestion might not be the best. Though personally I wouldn't mind that.

Do we really need to put these safeguards? I say let PHP deal with the typecasting as it normally would and throw error where it would.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ADmad Even though I think it's ugly if we let Arrays through, the problem will be PEBCHAK ;-)

But you are right, we don't need so much safeguarding.
What do you think of it now?

Here a list of all related tests (for your convenience):
testBlockSet
testBlockReset
testBlockSetNull
testBlockSetObjectWithToString
testBlockSetObjectWithoutToString
testBlockSetDecimal

testBlockAppend
testBlockAppendNull
testBlockAppendObjectWithToString
testBlockAppendObjectWithoutToString
testBlockAppendDecimal

testBlockPrepend
testBlockPrependNull
testBlockPrependObjectWithToString
testBlockPrependObjectWithoutToString
testBlockPrependDecimal

Copy link
Member

Choose a reason for hiding this comment

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

I say remove all safeguards and let php handle the type casting to string and throw error when required.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done so, replaced the three exception expecting tests to check for PHPUnit_Framework_Error now.
http://phpunit.de/manual/current/en/writing-tests-for-phpunit.html#writing-tests-for-phpunit.errors

Hope it's OK now.

https://cakephp.lighthouseapp.com/projects/42648-cakephp/tickets/3515

I used (string) because it is faster than strval() and there are no real benefits in this case.
Also enabled assign(), append() and prepend() to take any values convertible to string.

Removed try/catch as discussed with ADmad.
Changed the three exception expecting tests to check for PHPUnit_Framework_Error now.
$this->View->append('testWithObjectWithToString', $objectWithToString);
$result = $this->View->fetch('testWithObjectWithToString');
$this->assertSame("Block I'm ObjectWithToString", $result);
}
Copy link
Member

Choose a reason for hiding this comment

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

Could you not replace the repetition in the tests with a dataprovider?

markstory added a commit that referenced this pull request Aug 30, 2013
Conflicts:
	lib/Cake/Test/Case/View/ViewTest.php

Fixes #3515
@markstory markstory merged commit 2a540ff into cakephp:2.4 Aug 30, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants