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

[com_fields] Field type Repeatable. Editor and Textarea subfields remove HTML since J 3.9.7 #25189

Merged
merged 7 commits into from Jun 14, 2019

Conversation

ReLater
Copy link
Contributor

@ReLater ReLater commented Jun 12, 2019

Pull Request for Issue #25187

Summary of Changes

  • Since Joomla 3.9.7 subform child fields are validated. So a minimum validation happens like known from "normal" form fields if no filter attribute is set for them.
  • Because the com_fileds repeatable field uses a subform HTML tags are removed e.g. from editor or textarea contents during validation. This PR adds a filter field to any subfield row of repeatable fields.

Testing Instructions

  • Create a new com_fields field for Articles of type repeatable .

  • Add a subfield of type Editor.

  • Edit an article and enter some paragraphs in that editor.

  • Save the article => paragraphs removed.

  • Apply patch.

  • Open above field.

  • You'll see a new filter field for any row but for rows with type Media or Number:

12-06-_2019_15-48-04

  • Set Filter "Text" or "Safe Html."

  • Save field.

  • Open and edit article again or use a new one and enter some paragraphs => Save article => paragraphs NOT removed.

  • Test if filters are applied and work as expected for any field types.

  • The filter options change depending on your selection in Type

@brianteeman
Copy link
Contributor

Wasn't this as a result of JSST @SniperSister @zero-24

@ReLater
Copy link
Contributor Author

ReLater commented Jun 12, 2019

Wasn't this as a result of JSST @SniperSister @zero-24

Yes. See edited first comment.

@alikon
Copy link
Contributor

alikon commented Jun 12, 2019

How can I convert a draft pr to a regular one on GitHub????

just click on ready for review

image

@ReLater ReLater marked this pull request as ready for review June 12, 2019 14:22
@ReLater
Copy link
Contributor Author

ReLater commented Jun 12, 2019

just click on ready for review

Thank you!

@ReLater ReLater changed the title Repeatable custom field: Editor and Textarea fields always filter HTML since J 3.9.8 [com_fields] Field type Repeatable. Editor and Textarea subfields fields remove HTML since J 3.9.7 Jun 12, 2019
@ReLater ReLater changed the title [com_fields] Field type Repeatable. Editor and Textarea subfields fields remove HTML since J 3.9.7 [com_fields] Field type Repeatable. Editor and Textarea subfields remove HTML since J 3.9.7 Jun 12, 2019
@lunalars
Copy link
Contributor

filter not making sense: maybe the filter should only be visible if editor or textarea is selected.
added your code manually and also added showon="fieldtype:editor[OR]fieldtype:textarea to fieldfilter and did some quick tests. seems to work for all selected fieldtypes.


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

@ReLater
Copy link
Contributor Author

ReLater commented Jun 12, 2019

New commit contains:

  • Added more filter options. Taken over from single field types text and textarea.
  • Added showon to filterfield (hide if media or number)
  • Added showon to options.

Code style OK??? I hate too long lines ;-)

@ReLater
Copy link
Contributor Author

ReLater commented Jun 13, 2019

@BPBlueprint @lunalars
Could you please test this pr and mark your tests unsuccessful or successful on https://issues.joomla.org/tracker/joomla-cms/25189 . It's easy to apply and remove the patch with extension "Joomla! Patch Tester".

Normally I lose interest after 1 or 2 months in my prs and fixing branch conflicts ;-) if I don't use features myself.

@lunalars
Copy link
Contributor

If I set fieldtype to "Editor" and filter to "Use settings from Plugin" the HTML tags are still removed - should it be like this?
other settings work just fine.


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

@lunalars
Copy link
Contributor

Sorry, forgot to mention: in the Editor Plugin (fields) Filter is set to "Text", so tags should not be removed.


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

@ReLater
Copy link
Contributor Author

ReLater commented Jun 13, 2019

Sorry, forgot to mention: in the Editor Plugin (fields) Filter is set to "Text", so tags should not be removed.

Yes, you're right.
Set in plugin "Fields - Editor" filter to "Text".
In custom repeatable field select Editor and set filter to "Use settings from plugin".
Setting is not respected.
(Added as "Known bug" to intro comment.)

Sorry, then I have to give up here without assistance of others.

@lunalars
Copy link
Contributor

Can "Use settings from Plugin" be removed here?
I think it's not clear wich settings are meant: from editor or repeatable plugin.

@ReLater
Copy link
Contributor Author

ReLater commented Jun 13, 2019

Can "Use settings from Plugin" be removed here?

Give it a try! ;-) I removed global setting "Use settings from plugin".

@lunalars
Copy link
Contributor

very nice :-)
One more: I get SimpleXMLElement::addAttribute(): Attribute already exists in plugins/fields/repeatable/repeatable.php on line 72
child attribute is already set on line 66

@lunalars
Copy link
Contributor

the mentioned warning appears on editing an article

@lunalars
Copy link
Contributor

I have tested this item ✅ successfully on 1824077

With the latest code selected filters work as expected. Tested all filters for text, textarea and editor.

The warning was gone after applying the latest patch correctly.


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

@BPBlueprint
Copy link

I have tested this item ✅ successfully.

I have tested text, textarea and editor with and without filters.
Works as expected.

@ghost
Copy link

ghost commented Jun 14, 2019

Status "Ready To Commit".

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 14, 2019
@HLeithner HLeithner merged commit a62756b into joomla:staging Jun 14, 2019
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 14, 2019
@HLeithner
Copy link
Member

thx

@HLeithner HLeithner added this to the Joomla 3.9.9 milestone Jun 14, 2019
@ReLater ReLater deleted the patch-1 branch June 14, 2019 10:29
@ReLater
Copy link
Contributor Author

ReLater commented Jun 14, 2019

Thank you, guys!

@@ -65,6 +65,11 @@ public function onCustomFieldsPrepareDom($field, DOMElement $parent, JForm $form
$child->addAttribute('name', $formField->fieldname);
$child->addAttribute('type', $formField->fieldtype);
$child->addAttribute('readonly', $readonly);

if (isset($formField->fieldfilter))
Copy link
Member

Choose a reason for hiding this comment

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

@ReLater
As @bembelimen told me there seems to be to problems in this PR, first why do you set JNO to "0"?
And isset($formField->fieldfilter) will in this case always be true.

I would suggest to set JNO to value="" and change isset() to !empty() also I think you have to cast it to (string) and maybe better compare it to !== ''.

@ReLater
Copy link
Contributor Author

ReLater commented Jun 19, 2019

And isset($formField->fieldfilter) will in this case always be true.

No. If you select type Media then fieldfilter is not set/is undefined and that was the reason for this line because you get a warning.

JNO=0 comes from the params that you can find in the params.xml of the plugins for single fields. E.g.
https://github.com/joomla/joomla-cms/blob/staging/plugins/fields/textarea/params/textarea.xml#L40

For me the question is: What effect should filter="0" have? Really no filtering (="unset"), fall back to "string" ... ? I don't know.

value="" was removed because it doesn't work. You can test that by setting a filter in the com_fields editor plugin. It's ignored in the repeatable field if you set "COM_FIELDS_FIELD_USE_GLOBAL".
https://github.com/joomla/joomla-cms/blob/staging/plugins/fields/textarea/params/textarea.xml#L39

TBH I never use com_fields and just provided this pr because we have submitted the security issue then and forgot to test com_fields repeatable and wanted to help concerned users quickly. @bembelimen can provide and describe a corrective pr if he thinks that something doesn't work here as expected.

@HLeithner
Copy link
Member

That's really strange but if it works it's ok for me. thx

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

9 participants