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

Fix deprecation notices Indexes annotation #2622

Merged
merged 3 commits into from
Apr 23, 2024

Conversation

jmikola
Copy link
Member

@jmikola jmikola commented Apr 17, 2024

Q A
Type improvement
BC Break no
Related issues #2235, #2249

Summary

The Indexes attribute/annotation was deprecated in doctrine/mongodb-odm@7435e66 for 2.2. There were deprecation messages added for using Indexes at the class level, and indexes within the Document annotation; however, it looks like Indexes on the property level was missed.

Also, one of the deprecation messages got screwed up in doctrine/mongodb-odm@3bdfe97. The message changed but the sprintf args weren't swapped to account for that.

I struggled to add tests for this because:

  • trigger_deprecation calls @trigger_error(), so output is silenced. I could theoretically use error_get_last() to check for the deprecation message, but there's no prior art for doing so. In fact, no deprecation messages seem to be tested.
  • The Indexes attribute cannot target a property, so the property-level deprecation message could only trigger when using it as annotation.
  • I couldn't discern the syntax for using Indexes as a class-level attribute with Index nested within. I consulted Support for nesting attributes with PHP 8.1 orm#9241 some some examples of nesting attributes, but all of my attempts resulted in exceptions (e.g. $value property of Indexes couldn't be set).

For manual testing of the annotation form, this is what I used:

/**
 * @ODM\Document
 * @ODM\Indexes({
 *   @ODM\Index(keys={"foo"="asc"})
 * })
 */
class DeprecatedIndexesAttributeOnClass
{
    /**
     * @ODM\Field(type="string")
     * @ODM\Indexes({
     *   @ODM\Index()
     * })
     */
    public string $foo;
}

Copy link
Member

@IonBazan IonBazan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we are missing tests for deprecation messages. Should we add them in this PR then?

@jmikola
Copy link
Member Author

jmikola commented Apr 18, 2024

I think we are missing tests for deprecation messages. Should we add them in this PR then?

I'll make an attempt to do so before wrapping this up. We have some prior art for doing this in PHPLIB, but Expecting Deprecations (E_USER_DEPRECATED) may have a better approach.

@jmikola
Copy link
Member Author

jmikola commented Apr 19, 2024

I made some progress in asserting deprecation messages, but ran into a roadblock attempting to use Indexes as an attribute (annotation is fine). I'll hold off on this until we can talk it through in the Slack thread.

@jmikola jmikola changed the base branch from 2.8.x to 2.7.x April 22, 2024 14:53
@jmikola
Copy link
Member Author

jmikola commented Apr 22, 2024

Manually rebased on 2.7.x, since that's the same branch I used for previous fixes (#2627).

@jmikola jmikola added this to the 2.7.1 milestone Apr 22, 2024
@jmikola jmikola added the Task label Apr 22, 2024
The argument order has been incorrect since the deprecation message was changed in 3bdfe97.
This was missed when Indexes was originally deprecated in 7435e66.
The attribute is intentionally untested. The Indexes attribute cannot target a property (it omits Attribute::TARGET_PROPERTY). When Indexes targets a class, Index attributes cannot be nested within its definition. Testing the "indexes" option for class-level Document attributes is technically possible but not worth the effort.
@jmikola
Copy link
Member Author

jmikola commented Apr 22, 2024

Deprecation messages for annotations are now tested. CI failures are unrelated to this PR and due to #2624.

@jmikola jmikola requested a review from IonBazan April 22, 2024 18:45
}

/** @param list<string>|null $errors */
private function captureDeprecationMessages(callable $callable, ?array &$errors): mixed
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can also use https://symfony.com/doc/current/components/phpunit_bridge.html but seems like a drastic change so let's keep it this way for now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aye. We use that on the MongoDB driver but I noted it wasn't being utilized here.

@jmikola jmikola merged commit e032256 into doctrine:2.7.x Apr 23, 2024
18 of 20 checks passed
@jmikola jmikola deleted the 2.8-indexes-deprecation branch April 23, 2024 13:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants