Skip to content

Fix not converted boolean in route::_ and log warning#25225

Merged
SniperSister merged 7 commits intojoomla:stagingfrom
HLeithner:jroute-workaround
Aug 8, 2019
Merged

Fix not converted boolean in route::_ and log warning#25225
SniperSister merged 7 commits intojoomla:stagingfrom
HLeithner:jroute-workaround

Conversation

@HLeithner
Copy link
Copy Markdown
Member

@HLeithner HLeithner commented Jun 16, 2019

Pull Request for Issue #25204 .

Summary of Changes

Added boolean conversion removed in pr #24089, also log a warning if wrong parameter type is used. Should be removed with J4.

Testing Instructions

use jroute::_ with $tls parameter as int and boolean (true/false)

Expected result

Now we convert it if type is Boolean.

Actual result

Boolean didn't got converted to 0 or 1.

@richard67
Copy link
Copy Markdown
Member

@HLeithner It seems you mixed up texts for "Expected result" and "Actual result" in your description. "Expected result" should describe result with this PR applied, "Actual result" should describe result of current staging, i.e. without this PR applied.

}

// @deprecated 4.0 Before 3.9.7 this method silently converted boolean to integer
if (is_bool($tls))
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is unnecessary, IMO. Strings and even arrays worked here too until recently. But argument has been documented as integer since at least 1.5.

@HLeithner
Copy link
Copy Markdown
Member Author

@SharkyKZ you are right it's unnecessary anyway I changed it to convert all variables to int.

@SharkyKZ
Copy link
Copy Markdown
Contributor

Just noticed this is in Route::_(). But it should be in Route::link().

@HLeithner
Copy link
Copy Markdown
Member Author

For me it's only a pre Joomla 3.9 thing (and should be removed in j4) because Route::link added in this version but I'm still not sure to add it as you already stated the documentation says it's an integer.

@SharkyKZ
Copy link
Copy Markdown
Contributor

I have tested this item ✅ successfully on 955c92a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25225.

@richard67
Copy link
Copy Markdown
Member

@kirblam Please test this at it handles your issue #25204 .

@ghost
Copy link
Copy Markdown

ghost commented Jul 17, 2019

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Aug 8, 2019

I have tested this item ✅ successfully on 955c92a


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25225.

@Quy
Copy link
Copy Markdown
Contributor

Quy commented Aug 8, 2019

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/25225.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Aug 8, 2019
@SniperSister SniperSister merged commit 0b13376 into joomla:staging Aug 8, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Aug 8, 2019
@SniperSister SniperSister added this to the Joomla! 3.9.11 milestone Aug 8, 2019
@HLeithner HLeithner deleted the jroute-workaround branch August 8, 2019 10:51
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.

7 participants