rsanchez commented 6 years ago

Using \Okta\Users\User::get to retrieve by an email with a plus sign in it yields an error. Plus is a valid (and common?) character in email addresses. This pull request converts fixes this issue.

Cache\Adapter\Common\Exception\InvalidArgumentException: Invalid key "dev_440852_oktapreview_com_api_v1_users_rob+okta+admin_robsanchez_com". Valid filenames must match [a-zA-Z0-9_\.! ]. in /Users/robsanchez/Sites/okta/vendor/cache/filesystem-adapter/FilesystemCachePool.php:145
Stack trace:
#0 /Users/robsanchez/Sites/okta/vendor/cache/filesystem-adapter/FilesystemCachePool.php(64): Cache\Adapter\Filesystem\FilesystemCachePool->getFilePath('dev_440852_okta...')
#1 /Users/robsanchez/Sites/okta/vendor/cache/adapter-common/AbstractCachePool.php(133): Cache\Adapter\Filesystem\FilesystemCachePool->fetchObjectFromCache('dev_440852_okta...')
#2 /Users/robsanchez/Sites/okta/vendor/cache/adapter-common/CacheItem.php(245): Cache\Adapter\Common\AbstractCachePool->Cache\Adapter\Common\{closure}()
#3 /Users/robsanchez/Sites/okta/vendor/cache/adapter-common/CacheItem.php(116): Cache\Adapter\Common\CacheItem->initialize()
#4 /Users/robsanchez/Sites/okta/vendor/cache/adapter-common/AbstractCachePool.php(161): Cache\Adapter\Common\CacheItem->isHit()
#5 /Users/robsanchez/Sites/okta/vendor/okta/sdk/src/DataStore/DefaultDataStore.php(261): Cache\Adapter\Common\AbstractCachePool->hasItem('dev_440852_okta...')
#6 /Users/robsanchez/Sites/okta/vendor/okta/sdk/src/DataStore/DefaultDataStore.php(150): Okta\DataStore\DefaultDataStore->executeRequest('GET', Object(GuzzleHttp\Psr7\Uri))
#7 /Users/robsanchez/Sites/okta/vendor/okta/sdk/src/Generated/Users/User.php(59): Okta\DataStore\DefaultDataStore->getResource('rob+okta+admin@...', 'Okta\\Users\\User', '/users')
#8 /Users/robsanchez/Sites/okta/plugins/okta/src/Service.php(99): Okta\Generated\Users\User->get('rob+okta+admin@...')

codecov-io commented 6 years ago

Codecov Report

Merging #49 into develop will not change coverage. The diff coverage is 100%.

@@            Coverage Diff             @@
##             develop      #49   +/-   ##
==========================================
  Coverage      95.01%   95.01%           
  Complexity       916      916           
==========================================
  Files            129      129           
  Lines           3148     3148           
==========================================
  Hits            2991     2991           
  Misses           157      157

Impacted Files	Coverage Δ	Complexity Δ
src/Cache/CacheManager.php	`100% <100%> (ø)`	`13 <0> (ø)`	:arrow_down:

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 c7f3f52...c2d89b2. Read the comment docs.

bretterer commented 6 years ago

@rsanchez thank you! Could you also update the test at https://github.com/okta/okta-sdk-php/blob/develop/tests/Unit/Cache/CacheManagerTest.php#L9-L24 to include testing the + is removed? Once done, Ill get this merged into Develop

rsanchez commented 6 years ago

@bretterer Thanks. I've updated the PR with test and rebased/squashed.

bretterer commented 6 years ago

Looks great.. Thank you for the contribution

bretterer commented 6 years ago

We will make sure this gets into our next release... Not sure of timeline, but you can always do a require of the develop branch. We strive to make develop always function, but as always, that is not 100% guarantee.

rsanchez commented 6 years ago

Hi, any news on a point release that includes this one? Thanks in advance.

okta / okta-sdk-php

Update CacheManager::createCacheKey() to strip plus signs #49

Codecov Report