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

Fixed #13497 returning "empty" values as null #13694

Merged
merged 3 commits into from
Jan 5, 2019
Merged

Fixed #13497 returning "empty" values as null #13694

merged 3 commits into from
Jan 5, 2019

Conversation

CameronHall
Copy link
Contributor

@CameronHall CameronHall commented Dec 24, 2018 •
edited
Loading

Hello!

In raising this pull request, I confirm the following (please check boxes):

  • I have read and understood the Contributing Guidelines?
  • I have checked that another pull request for this purpose does not exist.
  • I wrote some tests for this PR.

Small description of change: libmemcached no longer incorrectly returns "empty" values as null

Thanks

@CameronHall CameronHall force-pushed the bugfix/libmemcached-empty-values-as-null branch from 9531c64 to 607b062 Compare December 24, 2018 10:40
niden
niden requested changes Dec 24, 2018
View reviewed changes
phalcon/mvc/model/query.zep Show resolved Hide resolved
@CameronHall CameronHall force-pushed the bugfix/libmemcached-empty-values-as-null branch 2 times, most recently from c707fb4 to e2b086a Compare December 30, 2018 09:23
@CameronHall
Copy link
Contributor Author

This is ready for review @niden

@niden
Copy link
Member

niden commented Jan 3, 2019

@CameronHall Can I be a pain one more time please?

  • Rebase your work. There is a new section in the changelog where you can add your change.
  • Can you move the test to the relevant test stub in the Cache subfolder? Since this is I get empty value as null then my choice would be Cache\Backend\Libmemcached\GetCest. Create a new method there using the same naming as the one in there (small variation) and add the @issue in the docblock along with your name and email/github if you wish.

The LibmemcachedCest.php file will be refactored at some point so I want to not add to them. Also you can use the PhalconLibmemcached module to check memcached without using Phalcon and also you can create a trait under _data/fixtures/Traits/Cache to allow you to connect to libmemcached (same as the getCache() method).

@CameronHall CameronHall force-pushed the bugfix/libmemcached-empty-values-as-null branch from e2b086a to b195bb1 Compare January 5, 2019 01:28
@codecov
Copy link

codecov bot commented Jan 5, 2019

Codecov Report

Merging #13694 into 4.0.x will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##            4.0.x   #13694      +/-   ##
==========================================
+ Coverage   65.52%   65.52%   +<.01%     
==========================================
  Files         396      396              
  Lines       88175    88184       +9     
==========================================
+ Hits        57775    57785      +10     
+ Misses      30400    30399       -1
Impacted Files Coverage Δ
ext/phalcon/cache/frontend/igbinary.zep.c 7.01% <0%> (-0.39%) ⬇️
ext/phalcon/cache/frontend/msgpack.zep.c 6.34% <0%> (-0.32%) ⬇️
ext/phalcon/cache/backend/libmemcached.zep.c 85.62% <0%> (+0.49%) ⬆️
ext/phalcon/cache/frontend/none.zep.c 92.85% <0%> (+17.85%) ⬆️

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 52f8ce8...885b5e6. Read the comment docs.

@niden niden merged commit efc4e22 into phalcon:4.0.x Jan 5, 2019
@niden
Copy link
Member

niden commented Jan 5, 2019

@CameronHall I will fix the phpcs issues

sergeyklay
sergeyklay reviewed Jan 6, 2019
View reviewed changes
CHANGELOG-4.0.md
@@ -126,6 +126,7 @@
- Changed the `Phalcon\Session` namespace by refactoring the component. `Phalcon\Session\Manager` is now the single component offering session manipulation by using adapters. Each adapter implements PHP's `SessionHandlerInterface`. Available adapters are `Phalcon\Session\Files`, `Phalcon\Session\Libmemcached`, `Phalcon\Session\Noop` and `Phalcon\Session\Redis`. [#12833](https://github.com/phalcon/cphalcon/issues/12833), [#11341](https://github.com/phalcon/cphalcon/issues/11341), [#13535](https://github.com/phalcon/cphalcon/issues/13535)
- Fixed `Phalcon\Mvc\Models` magic method (setter) is fixed for arrays [#13661](https://github.com/phalcon/cphalcon/issues/13661)
- Fixed `Phalcon\Mvc\Model::skipAttributes` and `Phalcon\Mvc\Model::allowEmptyColumns` allowEmptyStrings & skipAttributes repsect the column mapping. [#12975](https://github.com/phalcon/cphalcon/issues/12975), [#13477](https://github.com/phalcon/cphalcon/issues/13477)
- Fixed `\Phalcon\Cache\Backend\Libmemcached` returning "empty" values being as `null` when they could be `0`, `false` or empty `string`. [#13497](https://github.com/phalcon/cphalcon/issues/13497)
Copy link
Contributor

Choose a reason for hiding this comment

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

@CameronHall, @niden why do you use previous version fro this change? Did you see [4.0.0-alpha.2] at the top of this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is my mistake @sergeyklay. I rebased the changelog without checking the output/changes. I'll fix it in another PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looks like it's been fixed anyway. Thanks @niden.

@niden niden added the documentation Documentation required label Apr 9, 2019
@CameronHall CameronHall deleted the bugfix/libmemcached-empty-values-as-null branch October 24, 2019 11:36
@niden niden added the 4.0 label Dec 2, 2019
@niden niden removed the documentation Documentation required label Dec 15, 2019
@niden niden added bug A bug report status: low Low and removed Bug - Low labels Dec 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sergeyklay sergeyklay sergeyklay left review comments

@niden niden niden requested changes

Assignees
No one assigned
Labels
bug A bug report status: low Low
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@CameronHall @niden @sergeyklay