googleapis/google-cloud-php

Up-to-date usage of the latest Cloud Firestore for PHP package triggers deprecation warnings

levacic opened this issue · 6 comments

levacic commented

Environment details

The issue seems unrelated to the OS or PHP version, though.

Steps to reproduce

  1. Retrieve a document from Firestore
  2. Set some field values
  3. Deprecation notice is triggered for class Google\Cloud\Firestore\WriteBatch

Code example

use Google\Cloud\Firestore\FirestoreClient;

$firestore = new FirestoreClient();

$document = $firestore->document('users/john');

$document->set(
    [
        'foo' => 'bar',
    ],
    [
        'merge' => true,
    ],
);

Explanation

The deprecation notice is caused while checking for the existence of the deprecated Google\Cloud\Firestore\WriteBatch class within Google\Cloud\Firestore\DocumentReference::batchFactory(), before an attempt to set WriteBatch as an alias for the newer Google\Cloud\Firestore\BulkWriter class.

The class_exists() check triggers autoloading of the WriteBatch class, whose code in turn triggers the deprecation notice as soon as the file is loaded.

The WriteBatch class/file also attempts to configure the same alias, and that's probably the statement that actually has an effect, because it happens before the class_alias() call within DocumentReference occurs.

This generates a deprecation notice within any request/invocation that attempts to work with a document, which gets really spammy in logs.

Unfortunately, there's not much users can do to address this, as the deprecation notice is triggered by the code in the package. It would be possible to configure the alias by the user before the Firestore package is used, but that seems needlessly hacky.

Furthermore, the deprecation notice states that WriteBatch will be removed in the next major release - but considering the current v1.47.0 version of the package, a major release that removes the deprecation notice probably won't happen soon.

Solution

A possible solution would be to just pass false as the second argument to class_exists(), which will disable the autoloading attempt in case the class hasn't been defined/loaded yet.

It doesn't seem like this would have any negative effects, as the WriteBatch alias will be immediately created anyway.

The same issue probably exists in Google\Cloud\Firestore\FirestoreClient::batch(), though I haven't personally interacted with that method directly.

bshaffer commented

try using the latest release that went out today (v1.47.1).

Woops, @levacic you/re right, I meant to link https://github.com/googleapis/google-cloud-php-firestore/releases/tag/v1.47.2

levacic commented

Thanks for the quick reply!

Are you sure that's the right version, though? The link you shared states that this version was released on Nov 6:

Screenshot 2024-12-12 at 23 10 39

I also can't find a newer version (in that repo, or on Packagist).

levacic commented

Unfortunately, I am not able to currently test this upgrade on the project affected by the issue (though I will in the near future).

The reason is that, even though upgrading from v1.47.0 to v1.47.2 should be a patch upgrade, these changes trigger upgrades of other dependencies, some of which should in practice be major - e.g. through a chain of these required dependency upgrades (google/gax -> google/auth), it turns out that the patch upgrade of google/cloud-firestore also requires me to upgrade from psr/log:^2 to psr/log:^3, which is then blocked by some other dependencies in my project.

Overall, it doesn't seem that these Google Cloud PHP packages strictly adhere to semantic versioning, despite the claim in the README. As a trivial example, upgrading google/auth from v1.37.0 to v1.38.0 drops support for PHP 7.4 which is definitely a major BC break - even though the package version only got a minor version bump.

Regardless though, by just reviewing the actual code changes between google/cloud-firestore:v1.47.0 and google/cloud-firestore:v1.47.2, it doesn't seem to me like the cause has been addressed at all - the same code that triggers the deprecation warnings is still present in the package, and hasn't been changed at all.

Could you perhaps review the actual solution I've proposed in the initial issue description, and maybe apply that change if it makes sense?

bshaffer commented

@levacic dropping support for versions of PHP are NOT considered a breaking change, and so our packages do adhere to semantic versioning. Here is a good article as to why that is

However, I see that the deprecation warnings you are referring to are not the ones which have been fixed in the latest version of this library... the fix in the latest version is for deprecation warnings when upgrading to PHP 8.4.

I'll take a look at the issue you've described and the solution you've recommended. I think the solution you've proposed (providing false to class_exists) will fix the issue. Thanks for helping us track down this issue!

levacic commented

@levacic dropping support for versions of PHP are NOT considered a breaking change, and so our packages do adhere to semantic versioning. Here is a good article as to why that is

That's an interesting take, and I'll have to think about it, although my gut feeling is that I don't totally agree with it, as I'd consider the advertised platform support (e.g. supported PHP versions in this case) a part of the package's public API. And my understanding of the intent of semantic versioning is that non-major upgrades should not require users to make changes (which is obviously not the case here).

On the other hand, the FAQ on the semver website itself seems to be in agreement with the blog post you've linked, so I'm not sure what to make of it.

Either way, appreciate the link!

However, I see that the deprecation warnings you are referring to are not the ones which have been fixed in the latest version of this library... the fix in the latest version is for deprecation warnings when upgrading to PHP 8.4.

I'll take a look at the issue you've described and the solution you've recommended. I think the solution you've proposed (providing false to class_exists) will fix the issue. Thanks for helping us track down this issue!

I saw you already made a PR to address this, thanks a lot! I believe the same change should be applied to FirestoreClient::batch() as well.

  • 👍1
bshaffer commented

And my understanding of the intent of semantic versioning is that non-major upgrades should not require users to make changes (which is obviously not the case here).

The way I think of it is the defining trait of a major version bump is if there's a breaking change. Dropping support for a version of the language runtime is not breaking as long as it's handled as expected by the package manager. In the case of PHP, this is handled by Packagist/Composer. Also, if dropping support for a requirement constituted a breaking change, then wouldn't removing / upgrading any dependencies also need to be considered a breaking change, following the same logic?

I suppose you could think of it like... for people on the previous version of PHP, there IS a major version bump - they have to upgrade to the next major version of the language, which could cause breaking changes. But for the other users of the library on the other version of PHP, there is no major version bump (as there were no breaking changes)

I believe the same change should be applied to FirestoreClient::batch() as well.

Thank you for catching that! I've added it to the PR as well.

  • 👍1