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

[4.0] Fixing switcher label for true/false #27098

Merged
merged 16 commits into from
Feb 16, 2020
Merged

Conversation

Quy
Copy link
Contributor

@Quy Quy commented Nov 18, 2019

Pull Request for Issue #26749.

Summary of Changes

In Global Configuration, 0/1 are saved as true/false. Read #26749 (comment)
false cast as string returns empty string and true returns 1.
Assign it 0 so it can match the option value 0.

			<option value="0">JNO</option>
			<option value="1">JYES</option>

Testing Instructions

Go to Global Configurations
See Site Offline missing label
Apply PR
Make sure switcher labels not missing

Open configuration.php
Toggle Site Offline
Make sure $offline is saved as true or false

@ChristineWk
Copy link

I have tested this item ✅ successfully on f17349d

Toggled Site Offline. configuration.php > public $offline=true;


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

@vaibhavsTekdi
Copy link
Contributor

I have tested this item ✅ successfully on f17349d

Working fine as per the mentioned "Testing instructions".


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

@SharkyKZ
Copy link
Contributor

RTC.


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Nov 19, 2019
@infograf768
Copy link
Member

@HLeithner
is that OK for you?

@HLeithner
Copy link
Member

I'm still not sure if this is the proper way to do this. But yeah if it's the best solution to resolve the problem. Shouldn't this be done in the model/formfield layer and not in the layout?

@HLeithner
Copy link
Member

Forget my comment I mixed 2 pr's it seams to be ok.

@wilsonge
Copy link
Contributor

In this specific case wondering if we actually should have this in the form. normally this is correct. However in the normal (non-switcher) radio field it may not be.

@Quy
Copy link
Contributor Author

Quy commented Nov 22, 2019

It is working now for switcher and non-switcher.

27098

@HLeithner
Copy link
Member

Changing this in the Jformfield would mean that this is valid for all abstracted classes, that means if I have a input field I get a '0' instead of an empty string correct? (if the function is not overridden)

I wouldn't expect this.

@Quy
Copy link
Contributor Author

Quy commented Dec 5, 2019

It is checking for a false value. Empty string does not apply.
To test, apply PR. See Path to Cache Folder field where it is empty. It does not display 0.

@HLeithner
Copy link
Member

thats true but if the value is false it would convert it also for a text field into '0' and not ''

@Quy
Copy link
Contributor Author

Quy commented Dec 6, 2019

It is a strict comparison for boolean and false. A string value will not be affected.

@Quy Quy changed the title [4.0] Fixing switcher label [4.0] Fixing switcher label for true/false Dec 30, 2019
@infograf768
Copy link
Member

@Quy
You mean that when we have:

			<option value="true">JYES</option>
			<option value="false">JNO</option> 

It would have no impact?

@Quy
Copy link
Contributor Author

Quy commented Jan 26, 2020

I am not at a computer to test but it should not be affected because it is string and not boolean.

@Quy
Copy link
Contributor Author

Quy commented Jan 26, 2020

Also this applies to the configuration file only when storing true/false values if I am not wrong. It has been a while since doing this PR. My memory not so good these days.

@Fedik
Copy link
Member

Fedik commented Jan 26, 2020

I wounder how it may have RTC 😉

Doing if ($this->value === false) globally in FormField is incorrect, it will affect every field.

That maybe better to do after #27202 will be merged

@brianteeman
Copy link
Contributor

Works perfectly for me. I dont see any issues.

@Fedik please post exactly where it breaks


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

@brianteeman
Copy link
Contributor

I have tested this item ✅ successfully on 72e779b


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

@brianteeman
Copy link
Contributor

I can't comment about the code but it solves the problem


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

@Fedik
Copy link
Member

Fedik commented Feb 2, 2020

please post exactly where it breaks

People tested it with switcher field, but this changes:

will affect every field

this may lead to unexpected behavior, when example some extension have a custom field,
which expect value to be boolean false, and we force it to be string '0'.

@infograf768 infograf768 removed the RTC This Pull Request is Ready To Commit label Feb 3, 2020
@infograf768
Copy link
Member

Took off RTC until @Fedik comment is taken care of.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 3, 2020
@infograf768
Copy link
Member

setting to pending


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

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 3, 2020
@brianteeman
Copy link
Contributor

@Fedik could you please provide a specific example

@Fedik
Copy link
Member

Fedik commented Feb 3, 2020

I do not have a specific example to demonstrate, it a logic assumption that comes from the code review.

If we need this hacky thing, then better to do it specifically for the switcher field, not global.

There a couple ways:
Or move this hacky code to the switcher layout itself layouts/joomla/form/field/radio/switcher.php
Or fix these two conditions to work correctly:

<?php
// Initialize some option attributes.
$checked = ((string) $option->value == $value) ? 'checked="checked"' : '';
$active = ((string) $option->value == $value) ? 'class="active"' : '';
$oid = $id . $i;

@Quy
Copy link
Contributor Author

Quy commented Feb 14, 2020

Is this better?

@Fedik
Copy link
Member

Fedik commented Feb 14, 2020

much better 😉

@wilsonge wilsonge merged commit 3229d87 into joomla:4.0-dev Feb 16, 2020
@wilsonge
Copy link
Contributor

I mean it's grim. But much better than the initial code change. So merging. Thankyou!

@wilsonge wilsonge added this to the Joomla 4.0 milestone Feb 16, 2020
@Quy Quy deleted the switcher-label branch February 16, 2020 15:42
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