Quick code review / suggestions

[hitrov]

hi, as a developer of a package that became quite popular recently with almost 200k downloads on Packagist: https://packagist.org/packages/hitrov/oci-api-php-request-sign

used mostly in this repository, which has 400+ stars: https://github.com/hitrov/oci-arm-host-capacity

hope I can get your attention to some parts :)

• The README states that PHP 8.0 is required, but the composer.json file specifies >=5.6.
• ...so that It would be beneficial to use proper typing instead of relying on PHP 5.6 or even 7.4, which both are no longer supported.
• The code has multiple declarations of classes/interfaces in many places, such as in src/Oracle/Oci/Common/Auth/AuthProviderInterface.php.
• There are some classes with a very high number of lines of code, such as src/Oracle/Oci/ObjectStorage/ObjectStorageClient.php, which has 3,500 lines per class.
• There is a lot of repetitive caching code in src/Oracle/Oci/Common/Auth/CachingRequestingAuthProvider.php that could benefit from being decorated.
• Hardcoding certain things, like Region::$AP_CHUNCHEON_1 = new Region("ap-chuncheon-1", "yny", Realm::getRealm("oc1")), may require manual code adjustments every time (JSON configuration with automatic repository population - e.g. whenever new region introduced - could work instead).
• The installation via composer repo is using dev-master, but the actual branch is called main.
• The code is missing tests (directory) completely, which is a concern, that’s why:…
• The examples in the README all return 404 errors https://github.com/oracle/oci-php-sdk/blob/main/tests/Oracle/Oci/Examples.
• The use of block comments, such
  • public function getTenancyId() // : string
  • /*string*/ protected $private_key; instead of the more common PHP block comments, makes the documentation more complicated.
• Catching root \Exception is not good practice.

Additionally, it is unclear to me what it is intended to do beyond file uploads (examples are missing). For example, using my package I can create/list instances, browse namespaces etc. If I were a PHP developer with a need to use OCI Object Storage for business purposes, I would prefer to use the AWS PHP package with S3 compatible API.

Thanks for your attention!

[KartikShrikantHegde]

Hi @hitrov , Thanks for raising an issue. Please see my answers below:

• The code has multiple declarations of classes/interfaces in many places, such as in src/Oracle/Oci/Common/Auth/AuthProviderInterface.php.

Those classes/interfaces are designed to support future auth types. Those auth types are not currently in PHP SDK. Right now, the auth we’re currently supporting is Instance Principal and we will add more in the future.

• There are some classes with a very high number of lines of code, such as src/Oracle/Oci/ObjectStorage/ObjectStorageClient.php, which has 3,500 lines per class.

These classes are auto-generated and not meant to be edited/reviewed manually. They contain all the APIs Object Storage service supports. It will likely grow in the future as we start supporting them and other services.

• There is a lot of repetitive caching code in src/Oracle/Oci/Common/Auth/CachingRequestingAuthProvider.php that could benefit from being decorated.

Sure, We will consider removing those to make the code look tidy.

• Hardcoding certain things, like Region::$AP_CHUNCHEON_1 = new Region("ap-chuncheon-1", "yny", Realm::getRealm("oc1")), may require manual code adjustments every time (JSON configuration with automatic repository population - e.g. whenever new region introduced - could work instead).

The region is hard-coded in the PHP SDK. We’ll add the feature to read the region information from a JSON file – this feature exists in other OCI SDKs today.

• The installation via composer repo is using dev-master, but the actual branch is called main.

We will look into this, right now we are not sure what is causing the issue, we’ve attached a version tag (0.1.0) and should be able to retrieve the correct tag through composer.

• The code is missing tests (directory) completely, which is a concern, that’s why:…

We have test coverage internally and will continue adding more to cover the code. We’ll publish those test files here in the near future.

• The examples in the README all return 404 errors https://github.com/oracle/oci-php-sdk/blob/main/tests/Oracle/Oci/Examples

We will be fixing this link pretty soon.

• The use of block comments, such public function getTenancyId() // : string /string/ protected $private_key; instead of the more common PHP block comments, makes the documentation more complicated.

Thanks for the inputs, we’ll try fixing those and use a more standard way.

• Catching root \Exception is not good practice.

Yes, agree, and we’ll make improvements to our error handling.

Overall, thanks for looking into our new release and providing valuable feedback. We had customers request native support for Object Storage service in PHP SDK vs AWS S3 compatible API. Thus, we’re currently only supporting the object storage service and will likely be adding more OCI services as and when there is a need and our roadmap evolves. This is still an early stage oci-php-sdk and feedback is highly valued, and we’ll keep making improvements on this project.

[hitrov]

Hello @KartikShrikantHegde ,

Thank you for providing your responses!

To clarify my previous question, I was referring to the practice of having a single class or interface declaration in a file, which is a commonly accepted approach.