Skip to content

[staging] [fields plugins] Deleting use of Telephone filter as it is broken#30142

Closed
infograf768 wants to merge 3 commits intojoomla:3.10-devfrom
infograf768:fields_telephone
Closed

[staging] [fields plugins] Deleting use of Telephone filter as it is broken#30142
infograf768 wants to merge 3 commits intojoomla:3.10-devfrom
infograf768:fields_telephone

Conversation

@infograf768
Copy link
Copy Markdown
Member

@infograf768 infograf768 commented Jul 20, 2020

Pull Request for Issue #18566

Summary of Changes

The TEL Form filter being totally broken ( https://github.com/joomla/joomla-cms/blob/staging/libraries/src/Form/Form.php#L1466-L1544 ), the filter should never be used in the fields parameters.

Changing telephone filter tel to text in sql upgrade #__fields table for B/C.
Taking off the Telephone filter in the fields plugin parameters.

Testing Instructions

Create 3 fields (type text, textarea, repeatable->text
Use the Telephone filter => number always get an unwanted period when saved when editing an article, various formats are not honored.

Patch and try to create new fields of the same type (no need to save them)
The filter field telephone is no more proposed.

Use the new sql update => all existing fields which have been set with the tel filter are modified to use the text filter.

@richard67
I only created the sql update for mysql. Please add for postgresql and sqlazure

@infograf768 infograf768 mentioned this pull request Jul 20, 2020
Closed
@richard67 richard67 mentioned this pull request Jul 20, 2020
Merged
@richard67
Copy link
Copy Markdown
Member

richard67 commented Jul 20, 2020

Please add for postgresql and sqlazure

Done, see infograf768#56 .

@infograf768
Merge pull request #56 from richard67/staging-infograf768-fields_tele…
a45258d 
…phone-mod-1

[CMS PR joomla#30142] Add update sql for postgresql and sqlazure
@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Jul 20, 2020

Done. Tks.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jul 20, 2020

It seems the language string can either be removed or be marked as deprecated, not sure what's the right way.

As far as I could see it is only used at places where it is removed with this PR:

richard@vmkubu02:~/lamp/public_html/joomla-cms-4.0-dev$ find ./ -type f -name "*\.*" -exec grep -iHn "JLIB_FILTER_PARAMS_TEL" {} \;
./administrator/language/en-GB/lib_joomla.ini:317:JLIB_FILTER_PARAMS_TEL="Telephone"
./plugins/fields/text/text.xml:39:                                      <option value="tel">JLIB_FILTER_PARAMS_TEL</option>
./plugins/fields/text/params/text.xml:20:                               <option value="tel">JLIB_FILTER_PARAMS_TEL</option>
./plugins/fields/textarea/textarea.xml:62:                                      <option value="tel">JLIB_FILTER_PARAMS_TEL</option>
./plugins/fields/textarea/params/textarea.xml:41:                               <option value="tel">JLIB_FILTER_PARAMS_TEL</option>
./language/en-GB/lib_joomla.ini:317:JLIB_FILTER_PARAMS_TEL="Telephone"
richard@vmkubu02:~/lamp/public_html/joomla-cms-4.0-dev$

@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Jul 20, 2020

One never knows what one does with some core strings. Also, let's hope someone, one day, when on booze, corrects the TEL filter... 😸

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jul 20, 2020

I see, one could still use the filter from the library and the corresponding text in some 3rd party stuff. All fine.

@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Jul 23, 2020

@HLeithner
I guess this PR interests no one. Can you still get it in?

@HLeithner
Copy link
Copy Markdown
Member

HLeithner commented Jul 23, 2020

Why don't we fix it?

@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Jul 23, 2020

Good luck for that. The preg are very complex.
I have no idea who has done that code and it was never used in core until these fields were created.

@ghost
Copy link
Copy Markdown

ghost commented Sep 8, 2020

Use the new sql update

How?

@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Sep 8, 2020

Usually running the update query with PhpMyadmin, replacing #__fields by your db prefix.
But it looks like this PR is not going to make it...

@ghost
Copy link
Copy Markdown

ghost commented Sep 8, 2020

I have tested this item 🔴 unsuccessfully on a45258d

Filter "Telephone" deleted, but after SQL-Update:

  • "Text" and "Textarea" > "Use Settings from Plugin" (which is "Text")
  • "Repeatable" > "No"
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/30142.

@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Sep 8, 2020

Sorry, I do not understand your comment. Please post screenshots and results following also instrauctions in #18566

@ghost
Copy link
Copy Markdown

ghost commented Sep 8, 2020

Use the new sql update => all existing fields which have been set with the tel filter are modified to use the text filter.

  • Field Text and Textarea has Filter Use Settings from Plugin (not Text as written in Testing Instructions)
  • Field Repeatable has Filter No (not Text as written in Testing Instructions)

@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Sep 9, 2020

You are right and it looks like not fully.
What I found out.
Indeed when editing the field itself, it displays what you wrote.
But if you do not modify the field, when using the field when editing an article it uses the text filter.
It's weird indeed. I can't find the reason.

@zero-24
Copy link
Copy Markdown
Contributor

zero-24 commented Jun 12, 2022

@infograf768 I would say we should close this PR for J3 and not break other sites. For now we could start a new PR to fix the migration sql against j4.

@zero-24 zero-24 closed this Jun 12, 2022
@richard67
Copy link
Copy Markdown
Member

richard67 commented Jun 12, 2022

@infograf768 I would say we should close this PR for J3 and not break other sites. For now we could start a new PR to fix the migration sql against j4.

@zero-24 Who will do the new PR? I don't think @infograf768 is still available.

@richard67
Copy link
Copy Markdown
Member

richard67 commented Jun 12, 2022

@zero-24 P.S.: And should the issue #18566 be re-opened?

@infograf768
Copy link
Copy Markdown
Member Author

infograf768 commented Jun 13, 2022

And should the issue https://github.com/joomla/joomla-cms/issues/18566 be re-opened?

It should imho

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@infograf768 @richard67 @HLeithner @zero-24 @joomla-cms-bot