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

Minor changes on Stream adapter #14721

Merged
merged 7 commits into from Jan 17, 2020
Merged

Minor changes on Stream adapter #14721

merged 7 commits into from Jan 17, 2020

Conversation

@ekmst
Copy link
Contributor

ekmst commented Jan 17, 2020

Hello!

  • Type: code quality | enhancement | bugfix
  • Link to issue: #14725

In raising this pull request, I confirm the following:

  • 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
  • I have updated the relevant CHANGELOG
  • I have created a PR for the documentation about this change

Small description of change:

I could not reproduce this problem(#14714), but I think this will solve it.

Even if this does not help, I think this improvement will not be superfluous :)

Thanks

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 17, 2020

Codecov Report

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

@@            Coverage Diff             @@
##            4.0.x   #14721      +/-   ##
==========================================
+ Coverage   67.97%   67.97%   +<.01%     
==========================================
  Files         483      483              
  Lines      111473   111435      -38     
==========================================
- Hits        75773    75749      -24     
+ Misses      35700    35686      -14
@ruudboon

This comment has been minimized.

Copy link
Member

ruudboon commented Jan 17, 2020

@ekmst Can you add a test for this?

@ekmst

This comment has been minimized.

Copy link
Contributor Author

ekmst commented Jan 17, 2020

@ruudboon Existing tests pass and I did not add.

Ok, add later

@ekmst

This comment has been minimized.

Copy link
Contributor Author

ekmst commented Jan 17, 2020

I caught another bug when writing tests.
If we call the Stream::getKeys() method with empty data and using a prefix, we get an exception. The following is an example for this behavior:

$serializer = new \Phalcon\Storage\SerializerFactory();

$adapter = new \Phalcon\Cache\Adapter\Stream(
    $serializer,
    [
        'storageDir' => '/tmp/',
        'prefix'     => 'pref-',       // The '/tmp/pref-' directory will not be created until we have written any data
    ]
);
$adapter->getKeys();  // Throws an exception

We get an exception:

Fatal error: Uncaught UnexpectedValueException: RecursiveDirectoryIterator::__construct(/tmp/pref-/): failed to open dir: No such file or directory in ...
@ruudboon

This comment has been minimized.

Copy link
Member

ruudboon commented Jan 17, 2020

I caught another mistake when writing tests.
If we call the Stream::getKeys() method with empty data and using a prefix, we get an exception. The following is an example for this behavior:

$serializer = new \Phalcon\Storage\SerializerFactory();

$adapter = new \Phalcon\Cache\Adapter\Stream(
    $serializer,
    [
        'storageDir' => '/tmp/',
        'prefix'     => 'pref-',       // The '/tmp/pref-' directory will not be created until we have written any data
    ]
);
$adapter->getKeys();  // Throws an exception

We get an exception:

Fatal error: Uncaught UnexpectedValueException: RecursiveDirectoryIterator::__construct(/tmp/pref-/): failed to open dir: No such file or directory in ...

Let's handle that in a new issue.

@ekmst

This comment has been minimized.

Copy link
Contributor Author

ekmst commented Jan 17, 2020

Let's handle that in a new issue.

@ruudboon
Done

Copy link
Member

ruudboon left a comment

@ekmst Thank you

@ruudboon ruudboon requested a review from niden Jan 17, 2020
@ruudboon

This comment has been minimized.

Copy link
Member

ruudboon commented Jan 17, 2020

Please don't merge this yet. Looks like Travis missed this pull

@ekmst

This comment has been minimized.

Copy link
Contributor Author

ekmst commented Jan 17, 2020

@ruudboon
In the last commit, only the change log was updated, so I explicitly skipped the building([skip ci]).

The build was in the previous commit
https://ci.appveyor.com/project/sergeyklay/cphalcon/builds/30181621
and
https://travis-ci.org/phalcon/cphalcon/builds/638377382?utm_source=github_status&utm_medium=notification

@ruudboon

This comment has been minimized.

Copy link
Member

ruudboon commented Jan 17, 2020

@ekmst Sorry. I was sleeping ;)

@ekmst

This comment has been minimized.

Copy link
Contributor Author

ekmst commented Jan 17, 2020

@ruudboon no problem :)

@niden
niden approved these changes Jan 17, 2020
@niden niden merged commit 773fc81 into phalcon:4.0.x Jan 17, 2020
8 checks passed
8 checks passed
Build Phalcon Pecl Package Build Phalcon Pecl Package
Details
phpcs
Details
PHP 7.2 Test on ubuntu-latest PHP 7.2 Test on ubuntu-latest
Details
PHP 7.3 Test on ubuntu-latest PHP 7.3 Test on ubuntu-latest
Details
PHP 7.4 Test on ubuntu-latest PHP 7.4 Test on ubuntu-latest
Details
PHP 7.2 Test on macOS-latest PHP 7.2 Test on macOS-latest
Details
PHP 7.3 Test on macOS-latest PHP 7.3 Test on macOS-latest
Details
PHP 7.4 Test on macOS-latest PHP 7.4 Test on macOS-latest
Details
@niden

This comment has been minimized.

Copy link
Member

niden commented Jan 17, 2020

Thank you @ekmst !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.