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

[3.9.1] [com_fields] Make sure disabled fields are not added to the request at all #22923

Merged
merged 3 commits into from
Mar 7, 2019

Conversation

zero-24
Copy link
Contributor

@zero-24 zero-24 commented Nov 2, 2018

Pull Request for Issue #22038 & #22519

Summary of Changes

Make sure disabled fields are not added to the request at all

Testing Instructions

  1. Create a custom field for your articles.
  2. Check that ACL is set to edit this field only at super user (this is the default) and set it for Editor to denied
  3. create an Article as an Editor (Usergroup)
  4. You can't save the Article because of an Error "Invalid Field"

Expected result

Error is gone

Actual result

image

Documentation Changes Required

none.

cc @laoneo Please let us know you opinion / technical insight here. As disabled should not be used for security we might also need to change more places too?

@coolcat-creations
Copy link
Contributor

I have tested this item ✅ successfully on 7abc845

Now the field works as expected, thank you!


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

@Quy
Copy link
Contributor

Quy commented Nov 3, 2018

It does not work with media and repeatable types.

@zero-24
Copy link
Contributor Author

zero-24 commented Nov 4, 2018

It does not work with media and repeatable types.

What does not work? And what is different for that types?

@Quy
Copy link
Contributor

Quy commented Nov 4, 2018

I get the invalid field error message.

@laoneo
Copy link
Member

laoneo commented Nov 5, 2018

We added this function in #19884, which fixed some issues that checks couldn't be done if fields was loaded or not. Where exactly is the error thrown? Because the field should be added in a disabled state, so validate should not check that at all. This change is not fixing the cause. I think there is something wrong in the Controller or Form class itself.

@zero-24
Copy link
Contributor Author

zero-24 commented Nov 5, 2018

@BertaOctech
Copy link

I have tested this item 🔴 unsuccessfully on 7abc845

First I have created a custom field and I saw the error on article saving
I used a editor user trying to submit an article. The field had superuser permits
Then I applied the patch but nothing happened, the error was still there


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

@mbabker mbabker removed this from the Joomla 3.9.1 milestone Nov 23, 2018
@nonickch
Copy link
Contributor

nonickch commented Feb 9, 2019

Just run into this issue for the front-end profile.edit view in 3.9.2. I assume all components using custom fields with configurable permissions will have the same issue.

An alternative solution that seems to work for me is to set the field value to null. Either always or only when the field is disabled. But I don't really know why bool(false) is selected over null, nor why the missing field values are being injected, so I can't really offer a concrete opinion over this

@Roos-AID
Copy link

This problem is also manifesting itself in Edit User Profile when we have User Fields defined as Read Only through setting the permission on Edit Custom Field on Denied.
The fields are then shown in Edit User Profile, but as soon as the form is submitted, all Read Only fields are blanked out and made invalid.

I really hope this can be fixed in next drop.

@HLeithner
Copy link
Member

@zero-24 @laoneo whats the state of this PR?

@zero-24
Copy link
Contributor Author

zero-24 commented Feb 12, 2019

We are awaiting an suggestion / review by @laoneo to fix the root cause.

@HLeithner
Copy link
Member

thx

@HLeithner HLeithner added this to the Joomla 3.9.4 milestone Mar 7, 2019
@ggppdk
Copy link
Contributor

ggppdk commented Mar 7, 2019

Seems to work as desired,
also tested with checkboxes field and with multi-select field (list with multiple on)

Also the logic of this seems correct
you would add the presence of the field via "normalizing" code to detect empty fields that are not posted (checkboxes, multi-select, other??)
thus to make possible to submit empty values for them,
but when they are disabled (due to ACL) the proper thing is to keep their existing DB value and this works

@HLeithner
Copy link
Member

Thx for the test!

@zero-24 zero-24 deleted the disabled_fields branch March 11, 2019 16:38
@AndySDH
Copy link
Contributor

AndySDH commented May 10, 2020

when they are disabled (due to ACL) the proper thing is to keep their existing DB value and this works

Just a note, I found out that this does not work for the new Subfields

#24711

Meaning that saving a subfields type that contains a disabled/hidden child field (due to ACL), will empty the value for that child field, instead of keeping the existing value for it

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