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.4] Add support ID with 0 for value CheckboxField.php #37174

Open
wants to merge 22 commits into
base: 4.4-dev
Choose a base branch
from

Conversation

korenevskiy
Copy link
Contributor

@korenevskiy korenevskiy commented Mar 2, 2022

This amendment concerns to use this field to pass ID as an argument. If we use this field not in an XML file but in a dynamic table of fields. In each row of the table, we use the Checkbox to select a row number or select an ID in the list. After activating these checkboxes, we mark which of the parameter lists with which ID we have activated.
I believe that the original code implied the use of checkboxes as just a parameter in the static configuration of the settings page, not to select an ID, but simply to select certain settings.
Therefore, such a subtlety as using the value '0' as a value was not taken into account.
My change does not violate compatibility and does not violate the default settings.
And of course it would be stupid on the part of external developers to use the value '0' of the attribute to get an activated checkbox.
Because the value '0' is interpreted as an empty value and we get an active checkbox. In this way, it is stupid to mark activity using unreadable, this is a stupid approach to activate the checkbox.
Therefore, I suggest using the value '0' as the value for ID selection.

$field = JFormHelper::loadFieldType('сheckbox', true);
$field->setup(simplexml_load_string('<field type="check" default="0"  name="id" />', 0);
echo $field->getInput();

The result can only be seen in the generated HTML. Because of this limitation of fields, we can't check in any other way, except to look for values in HTML.
Before:

<div class="form-check form-check-inline">
	<input
		type="checkbox"
		name="check"
		id="id"
		value="1"
	>
</div>

After:

<div class="form-check form-check-inline">
	<input
		type="checkbox"
		name="check"
		id="id"
		value="0" 
	>
</div>

Here you need to search for the string value ="0" with tests, otherwise no way.
In the Checkbox field, any default value is rendering correctly except for 0.
The default value of 0 is rendering as 1.
This fix can be checked in any module add writing this XML example to the module’s XML configuration file and after check this value in the F12 panel in the browser .

<field 
	name="id"
	type="checkbox"  
	label="Test value 0 in Checkbox!!!"  
	default="0" />

@laoneo
Copy link
Member

laoneo commented Mar 19, 2022

Thanks for your pull request. Please add a proper test instruction, so we can test it.

@korenevskiy
Copy link
Contributor Author

Thanks for your pull request. Please add a proper test instruction, so we can test it.

I can't find the Checkbox field test file. I'm new to writing tests. Previously, I wrote only 2 amendments to Joomla tests. Please tell me where I can find the Checkbox field test.

@Quy
Copy link
Contributor

Quy commented Mar 22, 2022

Please provide steps to reproduce the issue in order to test the PR.

@korenevskiy
Copy link
Contributor Author

korenevskiy commented Mar 22, 2022

$field = JFormHelper::loadFieldType('сheckbox', true);
$field->setup(simplexml_load_string('<field type="check" default="0"  name="id" />', 0);
echo $field->getInput();

The result can only be seen in the generated HTML. Because of this limitation of fields, we can't check in any other way, except to look for values in HTML.
Before:

<div class="form-check form-check-inline">
	<input
		type="checkbox"
		name="check"
		id="id"
		value="1"
	>
</div>

After:

<div class="form-check form-check-inline">
	<input
		type="checkbox"
		name="check"
		id="id"
		value="0" 
	>
</div>

Here you need to search for the string value ="0" with tests, otherwise no way.
In the Checkbox field, any default value is rendering correctly except for 0.
The default value of 0 is rendering as 1.

@laoneo
Copy link
Member

laoneo commented Mar 23, 2022

When you have a look at #37349, there you can see some test instructions how it should be. If the tester has to edit some files, write that in the testing instructions. That's not a problem.

@korenevskiy
Copy link
Contributor Author

korenevskiy commented May 15, 2022

When you have a look at #37349, there you can see some test instructions how it should be. If the tester has to edit some files, write that in the testing instructions. That's not a problem.

You are right, I understood my mistake. I described a new correction indicating the perimers on the principle: as it was before, and how it will become in the future.
I added a description of the PR header .

@HLeithner HLeithner changed the base branch from 4.1-dev to 4.2-dev June 27, 2022 13:05
@HLeithner
Copy link
Member

This pull request has automatically rebased to 4.2-dev.

@joomla-bot
Copy link
Contributor

This pull requests has been automatically converted to the PSR-12 coding standard.

@Hackwar Hackwar added the Small A PR which only has a small change label Feb 26, 2023
@Quy Quy removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Mar 1, 2023
@Hackwar Hackwar added the bug label Apr 6, 2023
@HLeithner HLeithner changed the base branch from 4.2-dev to 4.3-dev May 2, 2023 16:30
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.3-dev.

@korenevskiy
Copy link
Contributor Author

@HLeithner Is it possible to transfer this PR to Joomla 5 ?

@Fedik
Copy link
Member

Fedik commented Jul 13, 2023

This is a bug, and can go to 4.x, just need to be tested

@Fedik

This comment was marked as outdated.

@Fedik
Copy link
Member

Fedik commented Jul 13, 2023

I have tested this item ✅ successfully on 8b05ef3


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

@korenevskiy
Copy link
Contributor Author

korenevskiy commented Jul 14, 2023

@Fedik How do you think if we set the Default field to FALSE, how should it work? And probably we think about in which cases it may turn out that we will need to specify the value FALSE.

<field type="check" default="false"  name="id" />

@Fedik
Copy link
Member

Fedik commented Jul 14, 2023

HTML supports only string value, so no boolean values.
You cannot do <input type="checkbox" value="false"> and expect it will be a boolean, it will be a string false. The same for Joomla XML fields.
so only 1 or 0

@korenevskiy
Copy link
Contributor Author

so only 1 or 0

You are right, this will be enough for all use cases.

@korenevskiy
Copy link
Contributor Author

@bembelimen Please confirm this PR

@HLeithner HLeithner changed the base branch from 4.3-dev to 4.4-dev September 30, 2023 22:45
@HLeithner
Copy link
Member

This pull request has been automatically rebased to 4.4-dev.

@korenevskiy
Copy link
Contributor Author

korenevskiy commented Dec 27, 2023

@HLeithner
And when will this PR be merged to v5?

@Hackwar Hackwar added the PBF Pizza, Bugs and Fun label Feb 21, 2024
@HLeithner HLeithner changed the title Add support ID with 0 for value CheckboxField.php [4.4] Add support ID with 0 for value CheckboxField.php Apr 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Maintainers Checked Used if the PR is conceptional useful PBF Pizza, Bugs and Fun PR-4.4-dev Small A PR which only has a small change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet