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 #11132 - trim warning in Cake\View\Helper::addClass #11133

Merged
merged 5 commits into from Sep 1, 2017
Merged

Fix #11132 - trim warning in Cake\View\Helper::addClass #11133

merged 5 commits into from Sep 1, 2017

Conversation

huwcbjones
Copy link
Contributor

Fixes #11132 .

Adds support for the class element of an $options array to also be an array.

@markstory markstory added this to the 3.5.2 milestone Aug 31, 2017
Copy link
Member

@markstory markstory left a comment

Choose a reason for hiding this comment

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

Having a test case for this case is needed. If you need help with that let me know and I can add a test.

@@ -197,7 +197,9 @@ protected function _confirm($message, $okCode, $cancelCode = '', $options = [])
*/
public function addClass(array $options = [], $class = null, $key = 'class')
{
if (isset($options[$key]) && trim($options[$key])) {
if (is_array($options[$key])) {
Copy link
Member

Choose a reason for hiding this comment

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

I think you need an isset() here as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was a quick patch - I'll have a look at writing a test.

Thank you for offering help - if I get stuck, I'll ask! :)

Copy link
Member

Choose a reason for hiding this comment

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

Thanks.

@dereuromark
Copy link
Member

This looks like sth that might need a test case to prevent regressions in the future.

if (isset($options[$key]) && trim($options[$key])) {
if (is_array($options[$key])) {
$options[$key][] = $class;
} else if (isset($options[$key]) && trim($options[$key])) {
Copy link
Member

Choose a reason for hiding this comment

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

elseif

@codecov-io
Copy link

codecov-io commented Aug 31, 2017

Codecov Report

Merging #11133 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master   #11133      +/-   ##
============================================
+ Coverage     94.88%    94.9%   +0.01%     
- Complexity    12850    12854       +4     
============================================
  Files           437      437              
  Lines         32767    32771       +4     
============================================
+ Hits          31092    31101       +9     
+ Misses         1675     1670       -5
Impacted Files Coverage Δ Complexity Δ
src/View/Helper.php 100% <100%> (ø) 19 <0> (+2) ⬆️
src/I18n/Parser/PoFileParser.php 100% <0%> (ø) 21% <0%> (+2%) ⬆️
src/Cache/Engine/FileEngine.php 90.05% <0%> (+1.1%) 73% <0%> (ø) ⬇️
src/Cache/CacheRegistry.php 100% <0%> (+4%) 11% <0%> (ø) ⬇️
src/Cache/CacheEngine.php 93.61% <0%> (+4.25%) 19% <0%> (ø) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95b0de3...2d7bd21. Read the comment docs.

public function testAddClassArray()
{
$Helper = new TestHelper($this->View);
$input = array('class' => [
Copy link
Member

Choose a reason for hiding this comment

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

Use short array syntax everywhere please.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed - thanks

@markstory markstory merged commit 736d89d into cakephp:master Sep 1, 2017
markstory added a commit that referenced this pull request Sep 1, 2017
@huwcbjones huwcbjones deleted the view_helper_trim branch September 1, 2017 23:09
o0h pushed a commit to o0h/cakephp that referenced this pull request Nov 16, 2017
o0h pushed a commit to o0h/cakephp that referenced this pull request Dec 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants