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

Update Constants.php #213

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Update Constants.php #213

wants to merge 3 commits into from

Conversation

dpDesignz
Copy link
Contributor

Update _isNULL and _notNULL constants to allow checks to run correctly.

Discovered that the isNull and isNotNull where conditions were not firing correctly due to incorrectly declared constants.

Update `_isNULL` and `_notNULL` constants to allow checks to run correctly
Uppercased right expression for type consistency specific to these methods in `isNull` and `isNotNull`
@codecov
Copy link

codecov bot commented Feb 7, 2022

Codecov Report

Merging #213 (e690998) into master (4bc6a25) will increase coverage by 0.46%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff              @@
##             master     #213      +/-   ##
============================================
+ Coverage     90.30%   90.76%   +0.46%     
  Complexity      838      838              
============================================
  Files            14       14              
  Lines          2083     2058      -25     
============================================
- Hits           1881     1868      -13     
+ Misses          202      190      -12     
Impacted Files Coverage Δ
lib/ezFunctions.php 89.09% <ø> (ø)
lib/Database/ez_pdo.php 91.44% <0.00%> (-1.61%) ⬇️
lib/Database/ez_sqlite3.php 90.76% <0.00%> (-0.77%) ⬇️
lib/Database/ez_sqlsrv.php 81.46% <0.00%> (+0.45%) ⬆️
lib/ezQuery.php 89.89% <0.00%> (+2.51%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4bc6a25...e690998. Read the comment docs.

@TheTechsTech
Copy link
Contributor

Update _isNULL and _notNULL constants to allow checks to run correctly.

Discovered that the isNull and isNotNull where conditions were not firing correctly due to incorrectly declared constants.

Those constants also suppose to build create the actual SQL query string format. There might not be enough or no tests covering them. The ezFunctionsTest.php does not actually touch any database.

Seems the Windows Github Actions Ci needs fixing now.

@dpDesignz
Copy link
Contributor Author

Update _isNULL and _notNULL constants to allow checks to run correctly.
Discovered that the isNull and isNotNull where conditions were not firing correctly due to incorrectly declared constants.

Those constants also suppose to build create the actual SQL query string format. There might not be enough or no tests covering them. The ezFunctionsTest.php does not actually touch any database.

Seems the Windows Github Actions Ci needs fixing now.

It failed the test when I left the $y value lowercase because it was doing an assertive array check in the test files, so I made them uppercase to match the values defined in the constants. I've been trying to work out for weeks why the NULL conditions weren't working but I only actually looked into it last night because I needed it for a query to run properly.

Copy link
Contributor

@TheTechsTech TheTechsTech left a comment

Choose a reason for hiding this comment

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

The naming of the constants and meaning actually represent contents without spacing.

Seems there might be some logic bugs elsewhere that should be looked at too. see

private function conditionIs($key, $condition, $combine)
and
private function processConditions($column, $condition, $value, $valueOrCombine, $extraCombine)

This construction will actually produced IS NOT NULL null the $y = 'null' not really needed.

@TheTechsTech
Copy link
Contributor

It failed the test when I left the $y value lowercase because it was doing an assertive array check in the test files, so I made them uppercase to match the values defined in the constants. I've been trying to work out for weeks why the NULL conditions weren't working but I only actually looked into it last night because I needed it for a query to run properly.

After looking a little further, there are other issues as mentioned, seems the need for more coverage tests should be done too.

Plus test under PHP 8.1 for compatibility, this part I been wanting to do for over 2 months now. Just not in the right mind to address issues, got many other things I'm working on.

@dpDesignz
Copy link
Contributor Author

The naming of the constants and meaning actually represent contents without spacing.

Seems there might be some logic bugs elsewhere that should be looked at too. see

private function conditionIs($key, $condition, $combine)

and

private function processConditions($column, $condition, $value, $valueOrCombine, $extraCombine)

This construction will actually produced IS NOT NULL null the $y = 'null' not really needed.

Yea I had originally tried removing this as well but it wouldn't parse without those values so that's why I used the solution that I have as that does parse with no issues.

Unfortunately, I'm in the same space as you otherwise I would have sorted this earlier myself.

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

2 participants