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

make HtmlHelper::tag() just return the $text content with no wrapping tag when $name === false #1292

Merged
merged 2 commits into from May 21, 2013

Conversation

openam
Copy link
Contributor

@openam openam commented May 20, 2013

If $name === false then there shouldn't be a wrapping tag.

@dereuromark
Copy link
Member

I must say I find that a little bit counter-intuitive. Especially since the documentation clearly states that it expects a string (@param string $name Tag name.).
So if anything, null or empty string would be the appropriate value to not returning a wrapping tag IMO.

@@ -896,6 +896,9 @@ public function tableCells($data, $oddTrOptions = null, $evenTrOptions = null, $
* @link http://book.cakephp.org/2.0/en/core-libraries/helpers/html.html#HtmlHelper::tag
*/
public function tag($name, $text = null, $options = array()) {
if ($name === false) {
Copy link
Member

Choose a reason for hiding this comment

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

Why not just use if (empty($name))? You can't do anything meaningful if the name is a non-empty string anyway.

Copy link
Member

Choose a reason for hiding this comment

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

Agreed, false/null/'' should all work the same ideally.

@openam
Copy link
Contributor Author

openam commented May 20, 2013

@dereuromark this was in reguards to the comments from @markstory on #1290. It's really about other helpers, e.g. PaginatorHelper, that use this. This allows for all of those to not force a wrapping tag.

@dereuromark
Copy link
Member

I see. Still dont like the idea of a non-stringish value to have some special treatment here - and think its better to use Admads approach using empy().

@openam
Copy link
Contributor Author

openam commented May 20, 2013

Shall I change it to (empty($name)) or just not do this at all? If we left it as $name === false we could throw an exception for when it's not a string.

@markstory
Copy link
Member

Thanks for taking the time to do this @openam :)

@dereuromark
Copy link
Member

👍

markstory added a commit that referenced this pull request May 21, 2013
make HtmlHelper::tag() just return the $text content with no wrapping tag when $name === false
@markstory markstory merged commit 28a3b73 into cakephp:master May 21, 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

4 participants