Closed srjlewis closed 7 months ago
Hello,
Thanks for your contribution, can you first fix the CI by running quality tools on your side please ?
PHPMD:
FILE: /home/travis/build/PHPSocialNetwork/phpfastcache/lib/Phpfastcache/Drivers/Couchbasev4/Config.php
------------------------------------------------------------------------------------------------------
13 | VIOLATION | The class Config has 50 public methods and attributes. Consider reducing the number of public items to less than 45.
13 | VIOLATION | The class Config has an overall complexity of 53 which is very high. The configured complexity threshold is 50.
PHPSTAN:
------ --------------------------------------------------------------------------------------
Line Phpfastcache/Drivers/Couchbasev4/Config.php
------ --------------------------------------------------------------------------------------
24 Property Phpfastcache\Drivers\Couchbasev4\Config::$servers type has
no value type specified in iterable type array.
π‘ See:
https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
37 Method Phpfastcache\Drivers\Couchbasev4\Config::getServers() return
type has no value type specified in iterable type array.
π‘ See:
https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
47 Method Phpfastcache\Drivers\Couchbasev4\Config::setServers() has
parameter $servers with no value type specified in iterable type
array.
π‘ See:
https://phpstan.org/blog/solving-phpstan-no-value-type-specified-in-iterable-type
201 Call to an undefined method
Couchbase\ClusterOptions::analyticsTimeout().
213 Call to an undefined method
Couchbase\ClusterOptions::bootstrapTimeout().
225 Call to an undefined method
Couchbase\ClusterOptions::connectTimeout().
237 Call to an undefined method
Couchbase\ClusterOptions::dnsSrvTimeout().
249 Call to an undefined method
Couchbase\ClusterOptions::keyValueDurableTimeout().
261 Call to an undefined method
Couchbase\ClusterOptions::keyValueTimeout().
273 Call to an undefined method
Couchbase\ClusterOptions::managementTimeout().
285 Call to an undefined method Couchbase\ClusterOptions::queryTimeout().
297 Call to an undefined method
Couchbase\ClusterOptions::resolveTimeout().
309 Call to an undefined method
Couchbase\ClusterOptions::searchTimeout().
321 Call to an undefined method Couchbase\ClusterOptions::viewTimeout().
333 Call to an undefined method
Couchbase\ClusterOptions::maxHttpConnections().
345 Call to an undefined method
Couchbase\ClusterOptions::configIdleRedialTimeout().
357 Call to an undefined method
Couchbase\ClusterOptions::configPollFloor().
369 Call to an undefined method
Couchbase\ClusterOptions::configPollInterval().
381 Call to an undefined method
Couchbase\ClusterOptions::tcpKeepAliveInterval().
393 Call to an undefined method
Couchbase\ClusterOptions::enableClustermapNotification().
405 Call to an undefined method
Couchbase\ClusterOptions::enableCompression().
417 Call to an undefined method Couchbase\ClusterOptions::enableDnsSrv().
429 Call to an undefined method
Couchbase\ClusterOptions::enableMetrics().
441 Call to an undefined method
Couchbase\ClusterOptions::enableMutationTokens().
453 Call to an undefined method
Couchbase\ClusterOptions::enableTcpKeepAlive().
465 Call to an undefined method Couchbase\ClusterOptions::enableTls().
477 Call to an undefined method
Couchbase\ClusterOptions::enableTracing().
489 Call to an undefined method
Couchbase\ClusterOptions::enableUnorderedExecution().
501 Call to an undefined method
Couchbase\ClusterOptions::useIpProtocol().
513 Call to an undefined method Couchbase\ClusterOptions::showQueries().
525 Call to an undefined method Couchbase\ClusterOptions::network().
537 Call to an undefined method
Couchbase\ClusterOptions::trustCertificate().
549 Call to an undefined method
Couchbase\ClusterOptions::userAgentExtra().
561 Call to an undefined method Couchbase\ClusterOptions::tlsVerify().
570 Parameter $options of method
Phpfastcache\Drivers\Couchbasev4\Config::setThresholdLoggingTracerOptions()
has invalid type Couchbase\ThresholdLoggingOptions.
570 Parameter $options of method
Phpfastcache\Drivers\Couchbasev4\Config::setThresholdLoggingTracerOptions()
has invalid type Couchbase\ThresholdLoggingOptions.
573 Call to an undefined method
Couchbase\ClusterOptions::thresholdLoggingTracerOptions().
582 Parameter $options of method
Phpfastcache\Drivers\Couchbasev4\Config::setTransactionsConfiguration()
has invalid type Couchbase\TransactionsConfiguration.
582 Parameter $options of method
Phpfastcache\Drivers\Couchbasev4\Config::setTransactionsConfiguration()
has invalid type Couchbase\TransactionsConfiguration.
585 Call to an undefined method
Couchbase\ClusterOptions::transactionsConfiguration().
------ --------------------------------------------------------------------------------------
------ -----------------------------------------------------------------------
Line Phpfastcache/Drivers/Couchbasev4/Driver.php
------ -----------------------------------------------------------------------
84 Class Couchbase\Cluster constructor invoked with 2 parameters, 1
required.
118 Caught class Couchbase\Exception\DocumentNotFoundException not found.
π‘ Learn more at https://phpstan.org/user-guide/discovering-symbols
159 Caught class Couchbase\Exception\DocumentNotFoundException not found.
π‘ Learn more at https://phpstan.org/user-guide/discovering-symbols
172 Method Couchbase\BucketManager::flush() invoked with 1 parameter, 0
required.
187 Call to an undefined method Couchbase\Cluster::diagnostics().
------ -----------------------------------------------------------------------
Hi
I have included all of the config options in the Config class which PHPMD over the edge with all the methords.
And you cant have Couchbase V3 and Couchbase V4 extentions loaded at the same time, so If you have any suggestion on how to do so would be helpful.
The only solution I see atm, is to add support of Couchbase v4 for the 9.2 release and deprecate the V3 as of 9.2. Which mean for now you need to include the v4 to the core by yourself using our API, do you know how to do it ?
Yes, that is how I am using this driver within my own project, just wanted to push the code back to yourself.
From now unfortunately its the best solution I can offer you. 9.2 will be probably released around end of January 2024. I need to deprecate v3 first so it can be safely removed as of v10.
Yep that is understandable, I was not after a solution.
from my side the driver plugins as using
CacheManager::addCustomDriver('Couchbasev4', \Framework\Core\Cache\Drivers\Couchbasev4\Driver::class);
Also for testing I do have a test for the forking problem which might help, it will need tweeking
CacheManager::addCustomDriver('Couchbasev4', Driver::class);
$config = (new Config())
->setUsername('username')
->setPassword('password')
->setBucketName('default')
->setServers(['10.10.10.10'])
->setUseStaticItemCaching(false);
$driver = CacheManager::getInstance('Couchbasev4', $config);
$cache = new Psr16Adapter($driver);
$value = random_int(0, 254);
$cache->set('key1', $value);
$pid = pcntl_fork();
if ($pid == -1) {
die('could not fork');
} else if ($pid) {
pcntl_wait($status);
} else {
exit($cache->get('key1'));
}
$this->assertTrue($value === $status / 256);
Did you found the "custom driver" API friendly to implement, just to be curious ? Because until today I had no feedback on this feature yet x)
I did not find it too bad personaly, but the documentation is lacking, I found reading the innerworkings was best.
I had to find out naming convention and namespace requirment of the Item and Config classes, and they need to exist by looking at the following https://github.com/PHPSocialNetwork/phpfastcache/blob/c82647aab9d7785eeb1414667649c091f3687eb1/lib/Phpfastcache/Core/Pool/DriverBaseTrait.php#L123-L130 https://github.com/PHPSocialNetwork/phpfastcache/blob/c82647aab9d7785eeb1414667649c091f3687eb1/lib/Phpfastcache/Core/Pool/DriverBaseTrait.php#L132-L139
I then had to find that config setters must start with "set" if you are extending ConfigurationOption class for the array loading to work. https://github.com/PHPSocialNetwork/phpfastcache/blob/c82647aab9d7785eeb1414667649c091f3687eb1/lib/Phpfastcache/Config/ConfigurationOption.php#L61-L72
From reading to docs the above information is not given, and coders who are lacking skills/time would struggle with impementing a custom driver. IMHO
And from a purly personal perspetive and I am not saying your choice is incorrect, but trying to be constructive. I would of not gone with
CacheManager::addCustomDriver($driverName, $className)
CacheManager::removeCustomDriver($driverName)
CacheManager::addCoreDriverOverride($driverName, $className)
CacheManager::removeCoreDriverOverride($driverName)
and gone with
CacheManager::addDriver($driverName, $className)
CacheManager::removeDriver($driverName)
and let the internals handle the difference between core and custom drivers, IMHO it is the job of phpfastcache to know the difference not implementation code, but this my personal preferance.
And from a purly personal perspetive and I am not saying your choice is incorrect, but trying to be constructive. I would of not gone with
Thanks for the feedback, I will update the WIKI next week to improve readability and understanding of the docs.
Hello,
I will wait for Phpstorm stubs to be added: https://github.com/JetBrains/phpstorm-stubs
Keep this branch active and allow maintainers to make changes as I will probably make bring changes on it in the next months.
At first I wanted to implement it for 9.2, but this will maybe for the V10 (around summer 2024).
In any case I need the stubs to be pushed (for PhpStan/PhpMD analysis).
I just have a question @srjlewis
By looking at the Couchbase documentation: https://docs.couchbase.com/php-sdk/current/project-docs/migrating-sdk-code-to-3.n.html
I feel like the Couchbase extension v4 is completely compatible with v4, are you sure that a new Driver should be made for it, there's no changes we can make to keep the current driver implementation compatible for v3 and v4 ?
Knowing that the current implementation is for the v3.2 at least (v3.2 brought support of scopes & collections):
SDK API 3.2: Introduced alongside Couchbase Server 7.0, provides features in support of Scopes and Collections, extends capabilities around Open Telemetry API to instrument telemetry data, enhanced client side field level encryption to add an additional layer of security to protect sensitive data, adds new platform support such as Ubuntu 20.04 LTS.
What's your minds on this ?
I'd really like to dig this topic deeper with you if you'd like so.
Thanks ! (and merry chrismas π π» )
There are slight differences.
I do agree it would be great to wrap both in a single driver.
Also there is a workaround in v4 diver for a limitation specific to the v4 extension, when used in a forking process.
one example is the DocumentNotFoundException
I see, for the exception class its indeed tricky to fool Phpstan π
What's the "forking process" exactly ?
Another question:
Would it be "easier" to make Couchbase_v4 extends Couchbase_v3 Driver/Config/Item class ?
So you only override needed properties/methods ? (Better maintainability).
Ok this was my thought process, hopefully it will makes sense. π€£
I will answaer the question first, I was going to extend the v3 clases, but the following stopped my.
I did not extend the Item class because it is a very small class and would be the same size.
The Config class from v4 has the potential of being used in both, but I have not checked the couchbase class compatibility between extentions.
use Couchbase\ClusterOptions;
use Couchbase\ThresholdLoggingOptions;
use Couchbase\TransactionsConfiguration;
As for the Driver class, I would say v4 as quite a few methods that are different (about 40-50%), so I thought that the changes where big enough to need it's own implementation, and allow potential future changes without making changes to v3
So back to the forking problem here is my bug report with couchbse (https://issues.couchbase.com/browse/PCBC-886) basically, with the current v4 extention if you make a connection to couchbase and then fork the php process then try to make a new connection within the child process and the run a command aginst couchbase it will block the call forever.
My workaround forces a change in the connection string using posix_getppid(), which forces the internals of the v4 extention to recreate a seperate connection to the parent and not use the persistent from the parent process. The https://github.com/PHPSocialNetwork/phpfastcache/pull/905#issuecomment-1834114211 as the the test for the forking problem.
I dont mind helping further with this and πMerry Chrismasπ
I did not want to introduce innter driver depenaces, that in the long run could couse problems, and keep each driver it's own entity. the v3 extention is not being developed any longer so the driver in its currrent state should not change. v4 driver has the potential to change has the extention develops. it make dropping v3 at a later point much eaiser.
That makes sense indeed.
As for the Driver class, I would say v4 as quite a few methods that are different (about 40-50%), so I thought that the changes where big enough to need it's own implementation, and allow potential future changes without making changes to v3
I think we can agree on one thing:
The v4 is very poorly documented by Couchbase enterprise: Missing source code from PHP-SDK (last version is still in v3.2): https://github.com/couchbase/php-couchbase So I can't even help myself by looking at C source code of the PHP Extension (π€)
Phpstorm stubs are still frozen at v3 despite the v4 extension being released more than a year ago:
Even the Phpstorm stubs are wronly documented as there's type error I just noticed today (π):
https://youtrack.jetbrains.com/issue/WI-75601/Couchbase-v4-stubs
The official Couchbase-PHP-SDK doc is not even mentioning new changes about exception hierarchy introduced in v4: https://docs.couchbase.com/php-sdk/current/project-docs/migrating-sdk-code-to-3.n.html#exception-handling So I can't even rely on the official SDK Doc from Couchbase (π).
So, for now, I can't take the risk of removing the v3 until the v4 is not yet properly documented/stabilized.
Keep your own implementation using internal Phpfastcache APIs while the v4 is maturing a bit more than it is right now.
We'll see next month if things have positively changed (I hope so...), meanwhile thank you for your time and code sharing.
Have a nice day srjlewis.
@Geolim4
The v4 extension code is now located at (https://github.com/couchbase/couchbase-php-client)
Which is a wrapper around the new core code (https://github.com/couchbaselabs/couchbase-cxx-client)
And I do agree the docs are bad, as you pointed out, the latest migration dosc are to v3 and there is not even any docs about migration to v4.
But I would stress major repos are not even shipping v3 of the extension and are only shipping v4, as the code for the v3 extension is not even compilable for php 8.3 without modification, so anyone using php 8.3 can only use the v4 extension.
I do know some repo's have patched their own version of the v3 extension for php 8.3, but I would think that's hit and miss at best between repo's.
I hope we can find a balance between the v3 and v4
Thanks
Anyway I need to wait that Couchbase release a Windows build on pecl since my personal coding computer is running on Windows despite my backend are running under a Unix VM.
And if you could help me to find reliable v4 stubs, I'd be grateful to have them under my hands π
In any case 8.3 has just being released so we still have some time before the v3 get completely deprecated.
Judging by the official PHP support timeline there no need to rush for now, right ?
Hi @Geolim4
Stubs are not required now with version v4 as the Couchbase Extention is now a hybrid extention. https://github.com/couchbase/couchbase-php-client/tree/main/Couchbase - PHP Part https://github.com/couchbase/couchbase-php-client/tree/main/src - C Extention Part which is a wrapper https://github.com/couchbaselabs/couchbase-cxx-client - Shared C client which gets wrapped for each language/platform (PHP, JAVA, Embedded, and more)
The only part of the Couchbase v4 namespace that is actually the C Extention which is \Couchbase\Extension
the rest is PHP code that is required using "couchbase/couchbase": "^4.0"
, so instead of needing stubs, it's satisfied via composer.
For you're information on the uptake side of things, I use EL8 adn EL9 distrubutions based from RedHat Linux Enterprise builds.
The main distros are Fedora, CentOS, Rocky Linux, Alma Linux, Oracle Linux and a few more, the admin's of these distro's will mostly use Remi Repo, I know Remi had stopped shipping the couchbase v3 extention with PHP 8.2 and PHP 8.3 and from PHP 8.2 onwards the couchbase v4 extention will automatically replace v3 by a dnf update
For PHP 8.2 and PHP 8.3 I asked remi to release couchbase v3 extention, but there are changes required within the yum.conf
file to block the automatic replacement by blocking the v4 extention.
https://forum.remirepo.net/viewtopic.php?id=4415 - PHP 8.2
https://forum.remirepo.net/viewtopic.php?id=4489 - PHP 8.3 patch required to compile
So within the RedHat Linux world Couchbase V3 extention is well on its way out of the door, as PHP 8.2 and 8.3 v4 is used by default.
I will admit I am not sure what the state of the Debian/Ubuntu distros are, on the windows side of things I would of thought they should of release a windows compile by now since v4 has been out for 1.5 years and they are not doing any updates on v3, but I have a feeling they might not release a windows compile or you will need to raise a bug with them (Couchbase).
It's a complete mess. I don't understand anymore what's Couchbase are doing. I'm getting tired to be required to update the whole code of Couchbase every 2 years. Every 2 years they refactor completely the client/library π
I agree very much with that, we use Couchbase quite a lot, I dont mind helping with the maintainace of the couchbase drivers, would you like me to update this driver ready for the changes you are planning in 9.2?
I think I will make it in a separate package as phpfastcache/couchbasev4-extension
.
I need to keep the couchbasev3 until the v10, especially since the couchbase v4 requires:
And none of them have Windowsd builds yet :(
And due to the extreme difficulty to make them cohabit on the lib with Couchbasev3, the better is to make a phpfastcache/couchbasev4-extension
in a separate sub-library.
I'll add a ExtensionManager
to the current 9.2 that will looks like:
\Phpfastcache\ExtensionManager::enableCouchbaseV4Driver
I will create a new repository for this extension and give you the rights on this one :)
That makes sense for now.
I created the repo: https://github.com/PHPSocialNetwork/couchbasev4-extension
Just a thing: Make sure that posix is loaded in "driverCheck()".
I added it in composer requirements, but make sure to make all the checks needed for now.
I know it is related to an issue you opened:
My workaround forces a change in the connection string using posix_getppid(), which forces the internals of the v4 extention to recreate a seperate connection to the parent and not use the persistent from the parent process. The https://github.com/PHPSocialNetwork/phpfastcache/pull/905#issuecomment-1834114211 as the the test for the forking problem.
But until this issue is fixed keep it as a prerequisite :)
Just a thing: Make sure that posix is loaded in "driverCheck()".
Yes agreed, I missed that one, I am just used to posix beening there.
I think it would be a idea to move/start a new discussion on the new repo. As we need to figure out how the structure for a extention, testing etc.
I disabled the issues on the new repo to keep it on the main repo (here).
But I can enable team discussion indeed.
Packagist has been created too: https://packagist.org/packages/phpfastcache/couchbasev4-extension
I gave you the maintainer rights as well.
I disabled the issues on the new repo to keep it on the main repo (here).
But I can enable team discussion indeed.
I am fine with ether way
Packagist has been created too: https://packagist.org/packages/phpfastcache/couchbasev4-extension
I gave you the maintainer rights as well.
I have just seen that, Thanks
To be honest it's a nice start for something I have in minds for long time now.
I always wanted to move specials drivers (Couchbase, Couchdb, Mongodb, etc) to a custom sub-repo with an extension mechanism to lighten the main Library.
It may be the start of something I plan for the v10, as a major internal rework of the lib.
I was going to suggest that, but I was being polite and did not want to step on peoples toes and hard work.
I was going to suggest that, but I was being polite and did not want to step on peoples toes and hard work.
Don't, its something that's been on my mind for years now, but it's a big rework indeed. v10 would be the perfect occasion for that.
I'm closing this one to continue on the discussion of PHPSocialNetwork/couchbasev4-extension π
Proposed changes
I have writen a driver to support Couchbase V4, with a work around for the forking problem within the current offical client (https://issues.couchbase.com/browse/PCBC-886).
Types of changes
What types of changes does your code introduce to Phpfastcache? Put an
x
in the boxes that applyAgreement
I have read the CONTRIBUTING and CODING GUIDELINE docs