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

Upgrade to Quickform 3.2.16 #224

Merged
merged 2 commits into from
Nov 29, 2018

Conversation

seamuslee001
Copy link
Contributor

This is a revised PR following discussion between myself and @colemanw. Following Coleman's idea i did a diff between the original version 3.2.11 and 3.2.16 and created this gist https://gist.github.com/seamuslee001/0a67ad08359f46b183a7e26f05c99658 of the difference. I left out the changes for security. I should also note that our version of hierselect.php is really really really old. I have therefor left it out of this PR as we need to consider it separately

@civibot
Copy link

civibot bot commented Jul 27, 2018

(Standard links)

@seamuslee001
Copy link
Contributor Author

Jenkins re-test this please

@@ -45,7 +45,7 @@ class HTML_QuickForm_Rule_Range extends HTML_QuickForm_Rule
* @access public
* @return boolean true if value is valid
*/
function validate($value, $options)
function validate($value, $options = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

change 2

Copy link
Contributor

Choose a reason for hiding this comment

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

So this is weird - it doesn't match the parent signature - but it didn't before either - feels smelly but I guess....

   /**
    * Validates a value
    *
    * @access public
    * @abstract
    */
    function validate($value)
    {
        return true;
    }

* @access private
* @return mixed
*/
function _findValue(&$values)
function _findValue(&$sc1 = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

change 7

Copy link
Contributor

Choose a reason for hiding this comment

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

Did we miss a trick here - it seems less compatible with the parent

    /**
     * Tries to find the element value from the values array
     *
     * @since     2.7
     * @access    private
     * @return    mixed
     */
    function _findValue(&$values)
    {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure that is the change but this is the direct comparison from 3.2.11 to 3.2.16 of the upstream https://gist.github.com/seamuslee001/0a67ad08359f46b183a7e26f05c99658#file-quickform_diff-diff-L748

* @access public
* @return void
*/
function accept(&$renderer)
function accept(&$renderer, $sc1 = false, $sc2 = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

change 8

Copy link
Contributor

Choose a reason for hiding this comment

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

Also not matching the parent

    * @param HTML_QuickForm_Renderer    renderer object
    * @param bool                       Whether an element is required
    * @param string                     An error message associated with an element
    * @access public
    * @return void
    */
    function accept(&$renderer, $required=false, $error=null)
    {
        $renderer->renderElement($this, $required, $error);
    } // end func accept

    // }}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* This is essentially a hidden element and should be rendered as one
*/
function accept(&$renderer)
/**
Copy link
Contributor

Choose a reason for hiding this comment

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

change 9

Copy link
Contributor

Choose a reason for hiding this comment

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

parent is ```
function accept(&$renderer, $required=false, $error=null)
{
$renderer->renderElement($this, $required, $error);
} // end func accept

// }}}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

* @access public
* @return void
*/
function accept(&$renderer)
function accept(&$renderer, $sc1 = false, $sc2 = null)
Copy link
Contributor

Choose a reason for hiding this comment

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

change 10

Copy link
Contributor

Choose a reason for hiding this comment

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

ditto sig

Copy link
Contributor

Choose a reason for hiding this comment

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

ah this might BE the parent!

Copy link
Contributor

Choose a reason for hiding this comment

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

no it's HTML_QuickForm_element

@@ -0,0 +1,159 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

change 13....

@eileenmcnaughton
Copy link
Contributor

@seamuslee001 so there is no chance we've missed our parent signature changes somehow.

The only thing I see is some possible function signature discrepancies - but they won't be quiet failures so I'm ok with merging as we can fix any resulting notices if we need to

@eileenmcnaughton eileenmcnaughton merged commit faa4b03 into civicrm:master Nov 29, 2018
@seamuslee001
Copy link
Contributor Author

Thanks @eileenmcnaughton

@seamuslee001 seamuslee001 deleted the quickform_16_upgrade branch November 29, 2018 05:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants