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.

Sign up for GitHub

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

Jump to bottom

[Core] Fix bug where $val=0 would pass as blank! #2626

Merged
merged 2 commits into from
Mar 17, 2017
Merged

[Core] Fix bug where $val=0 would pass as blank! #2626

merged 2 commits into from
Mar 17, 2017

Conversation

gluneau
Copy link
Contributor

@gluneau gluneau commented Mar 1, 2017 •
edited by ridz1208
Loading

This bug affected the upload capability of LorisForm for addFile()

cause:

    [recording_file] => Array
        (
            [name] => Screenshot from 2016-08-09 18:54:44.png
            [type] => image/png
            [tmp_name] => /tmp/phpNvwMAZ
            [error] => 0
            [size] => 261121
        )

@gluneau gluneau added the Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label) label Mar 1, 2017
@gluneau gluneau added this to the 17.0 milestone Mar 1, 2017
@gluneau gluneau requested a review from driusan March 13, 2017 17:39
ridz1208
ridz1208 requested changes Mar 14, 2017
View reviewed changes
php/libraries/NDB_BVL_Instrument.class.inc
@@ -1080,7 +1080,7 @@ class NDB_BVL_Instrument extends NDB_Page
$flag = true;
if (is_array($elvalue)) {
foreach ($elvalue AS $val) {
if ($val == "") {
if ($val === "") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@gluneau This change should also be done on line 1091 as the same comparison is made

@ridz1208
Copy link
Collaborator

@gluneau even tho this does fix the issue, it seems to be a change in behaviour. some code might be relying on it (http://php.net/manual/en/types.comparisons.php)

@ridz1208
Copy link
Collaborator

After a quick check across instruments, it seems like they would have similar returns for other elements

@ridz1208 ridz1208 self-assigned this Mar 14, 2017
@ridz1208 ridz1208 added Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix and removed Discussion Required PR or issue awaiting the resolution of a discussion between all involved parties labels Mar 14, 2017
@driusan
Copy link
Collaborator

driusan commented Mar 14, 2017

@ridz1208 This is still tagged as requested changes by you. It looks fine to me, is there anything wrong with it?

@ridz1208 ridz1208 removed the Needs Work PR awaiting additional changes by the author or contains issues that the author needs to fix label Mar 14, 2017
ridz1208
ridz1208 approved these changes Mar 14, 2017
View reviewed changes
Copy link
Collaborator

@ridz1208 ridz1208 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@driusan this is ready. take a look at it

@driusan driusan merged commit 6af0aa8 into aces:17.0-dev Mar 17, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ridz1208 ridz1208 ridz1208 approved these changes

@driusan driusan Awaiting requested review from driusan

Assignees

@driusan driusan

@ridz1208 ridz1208

Labels
Bug PR or issue introducing/requiring bug fixes (not mutually exclusive with the Feature label)
Projects
None yet
Milestone
17.0
Development

Successfully merging this pull request may close these issues.
3 participants
@gluneau @ridz1208 @driusan