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

Modules - number not text #18194

Merged
merged 2 commits into from Oct 5, 2017
Merged

Modules - number not text #18194

merged 2 commits into from Oct 5, 2017

Conversation

brianteeman
Copy link
Contributor

@brianteeman brianteeman commented Oct 2, 2017

Numerous options in the modules are for numbers but the xml field type is text. This pr changes them to the correct value of number.

This ensures that a user cannot enter a letter and only numbers. There are no b/c issues as only a numerical character could ever have worked and there is no change in the way the data is stored.

I spotted this when debugging a user's site where they put "five" in the count field instead of "5" and wondered why it didnt work. After this PR that isnt possible

Numerous options in the modules are for numbers but the xml field type is text. This pr changes them to the correct value of number.

This ensures that a user cannot enter a letter and only numbers. There are no b/c issues as only a numerical character could ever have worked and there is no change in the way the data is stored.

I spotted this when debugging a user's site where they put "five" in the count field instead of "5" and wondered why it didnt work. After this PR that isnt possible
@Quy
Copy link
Contributor

Quy commented Oct 2, 2017

I have tested this item ✅ successfully on af71c95

Code review


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

@chmst
Copy link
Contributor

chmst commented Oct 2, 2017

I have tested this item 🔴 unsuccessfully on af71c95

Tested successfully on some modules, but unsuccessfully on modules/mod_articles_category/mod_articles_category

Additionally there could be used a filter="integer".


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

@brianteeman
Copy link
Contributor Author

@chmst can you give some more information please as I cannot tell from your test what doesnt work

@chmst
Copy link
Contributor

chmst commented Oct 2, 2017

You have for the Field count type="numbertext". It doenst work, of course. only a typo :) , but should not be merged.

@brianteeman
Copy link
Contributor Author

That was already fixed af71c95

@chmst
Copy link
Contributor

chmst commented Oct 2, 2017

This was not visible when I startet testing. So I can repeat the tests.

@chmst
Copy link
Contributor

chmst commented Oct 2, 2017

I have tested this item ✅ successfully on af71c95

Repeated some tests and inspection.


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

@ghost
Copy link

ghost commented Oct 3, 2017

RTC after two successful tests.

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

@mbabker
3.8.1 ?

@brianteeman
Copy link
Contributor Author

There is no rush to get this merged. Today I will be reviewing components and plugins for the same

@infograf768
Copy link
Member

See my comment here:
#18199 (comment)

@brianteeman
Copy link
Contributor Author

See my reply #18199 (comment)

@mbabker mbabker added this to the Joomla 3.8.2 milestone Oct 5, 2017
@mbabker mbabker merged commit 68c22f1 into joomla:staging Oct 5, 2017
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Oct 5, 2017
@brianteeman
Copy link
Contributor Author

Thanks

@brianteeman brianteeman deleted the module-number branch October 5, 2017 12:17
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

6 participants