Skip to content

Conversation

@photodude
Copy link
Contributor

Updates the CMS Framework package to framework/filter 1.2.0 with Composer

Backports Framework changes: Allow arrays of numbers to be processed by clean() and associated tests.
joomla-framework/filter#13
Backports additional changes from framework/filter 1.2.0

  • Add array support to most filters
    • Fixes Array to String conversion notices in clean() for most cases
  • Updates pcre patterns for numbers
    • Fixes floats case to properly handle exponent type floats rather than just decimal floats
  • Adds new general cases to test and verify the array filter functionality
  • Code style: fixes missing comma at end of array lists.
    *Fixes PHP7 issues

Testing
https://docs.joomla.org/Testing_Joomla!_patches
Do a general check that the various existing forms in the CMS function as expected.

@photodude
Copy link
Contributor Author

@mbabker Here is the backport of the recent framework changes for the input filter clean() method and test.

@photodude
Copy link
Contributor Author

@RickR2H would you be willing to test this?

@RickR2H
Copy link
Member

RickR2H commented Dec 16, 2015

@photodude I'll test this if I can. What is the best procedure to test as I don't have in depth knowledge of the code as I'm only a frontend developer ;)

@photodude
Copy link
Contributor Author

I would just do a general check that the various existing forms in the CMS function as expected.
I'm not sure there are any easy tests beyond that which could have a "clean" method triggered for testing.

@photodude
Copy link
Contributor Author

@mbabker @wilsonge Updated to match changes in framework/filter 1.2.0

@photodude
Copy link
Contributor Author

@brianteeman Unlike @wilsonge 's PR #8734 for updating to the current the framework package. This filter class is used in the CMS core.
This should have consideration for merging for 3.5

@photodude photodude changed the title Backport FW change: num arrays process by clean() Backport FW filter package changes: allow arrays process by clean() Jan 21, 2016
@brianteeman
Copy link
Contributor

Not sure why that comment is addressed at me. I have no say on what gets merged or when

@photodude
Copy link
Contributor Author

I only Pinged you since You had asked in #8734 if that PR "need to be merged before 3.5"

@wilsonge
Copy link
Contributor

Merge this: photodude#1

Get some testers in here. I've gotta focus on the sql upgrade. But added a 3.5 blocker tag as even though the FW filter package isn't being used by core - let's not run into PHP 7 issues because 3rd parties are using the framework filter package :)

@photodude
Copy link
Contributor Author

@RickR2H would you still be willing to test this?

@Stevec4
Copy link
Contributor

Stevec4 commented Feb 25, 2016

I received an error after applying this patch.
Fatal error: Uncaught Error: Class 'ComposerAutoloaderInit205c915b9c7d3e718e7c95793ee67ffe' not found in C:\xampp\htdocs\sites\aviondemo\libraries\vendor\autoload.php:7 Stack trace: #0 C:\xampp\htdocs\sites\aviondemo\libraries\cms.php(33): require() #1 C:\xampp\htdocs\sites\aviondemo\administrator\includes\framework.php(23): require_once('C:\\xampp\\htdocs...') #2 C:\xampp\htdocs\sites\aviondemo\administrator\index.php(40): require_once('C:\\xampp\\htdocs...') #3 {main} thrown in C:\xampp\htdocs\sites\aviondemo\libraries\vendor\autoload.php on line 7
I applied the patch to a 3.5 beta site running php 7.0.2
Let me know if I can provide any more info.
Steve

@photodude
Copy link
Contributor Author

@Stevec4 Thanks for testing, I didn't create the part of this PR for the composer changes So I'll have to dig into what's going on.

I'm not sure, but I think this patch might need to be applied against Stagging, before the composer changes were added it would have been a test against 3.4.8 or possibly 3.5B2.
The composer changes and the PHP7 compatibility add some different dynamics.

I'll see if I can rebase this soon since there are some new merge conflicts.

photodude and others added 8 commits February 25, 2016 18:53
Backports Framework changes: Allow arrays of numbers to be processed by clean() 
joomla-framework/filter#13

* Adds basic first level processing of array's of numbers
  * Fixes Array to String conversion notices in clean() for number cases
* Updates pcre patterns for numbers
  * Fixes floats case to properly handle exponent type floats rather than just decimal floats
* Adds new general cases to test and verify the array of numbers functionality
* Code style: fixes missing comma at end of array lists.
Backports additional changes from Framework/filter 1.2.0
Fix the line breaks in checkAttribute() so it's properly on the boolean
Backports changes from framework/filter 1.2.0
Replaces deprecated JString with StringHelper
@photodude
Copy link
Contributor Author

@Stevec4 I've rebased to staging and fixed the Merge conflicts. I think the issue you reported should be fixed. I tested against Staging, So I'm not sure about success for testing from J3.5B2 and then applying the patch.

@Stevec4
Copy link
Contributor

Stevec4 commented Feb 26, 2016

@photodude I have re applied the patch to both a 348 install and a 3.50b2 install with no issues.
I checked several forms and they seem to function correctly.
Steve

@anibalsanchez
Copy link
Contributor

I have tested this item ✅ successfully on 23ca41a

Test OK


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

@photodude
Copy link
Contributor Author

@Stevec4 thanks for the test. Would you please go to the issue tracker and mark it as a successful test.
https://issues.joomla.org/tracker/joomla-cms/8681

@brianteeman
Copy link
Contributor

Manually added the test from @Stevec4

Marking RTC - thanks everyone


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

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 26, 2016
wilsonge added a commit that referenced this pull request Feb 26, 2016
Backport FW filter package changes: allow arrays process by clean()
@wilsonge wilsonge merged commit 5a6ac42 into joomla:staging Feb 26, 2016
@wilsonge wilsonge added this to the Joomla! 3.5.0 milestone Feb 26, 2016
@wilsonge
Copy link
Contributor

Merged -thanks all!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Feb 26, 2016
@photodude
Copy link
Contributor Author

Thanks Everyone. I appreciate all the work and testing.

@photodude photodude deleted the patch-5 branch February 26, 2016 19:18
'style',
'title',
'xml'
'xml',
Copy link
Contributor

Choose a reason for hiding this comment

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

Why did a comma got added here and just below ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I personally prefer it when you have multiple array elements. I don't think our code style policy defines either way...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a Joomla Coding styles option.
"When splitting array definitions onto several lines, the last value may also have a trailing comma."
I follow that the last value should have a trailing comma

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

9 participants