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 not converted boolean in route::_ and log warning #25225

Merged
merged 7 commits into from Aug 8, 2019

Conversation

HLeithner
Copy link
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
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.

@@ -74,6 +75,25 @@ public static function _($url, $xhtml = true, $tls = self::TLS_IGNORE, $absolute
$tls = self::TLS_DISABLE;
}

// @deprecated 4.0 Before 3.9.7 this method silently converted boolean to integer
if (is_bool($tls))
Copy link
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
Member Author

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

@SharkyKZ
Copy link
Contributor

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

@HLeithner
Copy link
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
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
Member

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

@ghost
Copy link

ghost commented Jul 17, 2019

@Quy
Copy link
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
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.

None yet

7 participants