Problem/Motivation

#2996789: Deprecate Drupal\field\Tests\EntityReference\EntityReferenceTestTrait discovered that some BTB tests were using a trait in a WTB namespace, so we deprecated that trait: Drupal\field\Tests\EntityReference\EntityReferenceTestTrait.

The deprecation included a @trigger_error().

This change deprecated the trait in both 8.7.x and 8.6.x.

This breaks tests for contrib trying to keep up with both a current (8.6.x) and a previous (8.5.x) core release, without any warning or recourse.

Proposed resolution

Remove the @trigger_error() for the 8.6.x deprecation of Drupal\field\Tests\EntityReference\EntityReferenceTestTrait.

Leave the rest of the deprecation in place.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#2 3002011_2.patch771 bytesMile23
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Mile23 created an issue. See original summary.

Mile23’s picture

Status: Active » Needs review
FileSize
771 bytes

EZ patch.

gabesullice’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +API-First Initiative

I ran JSON API's broken tests prior to applying this patch and confirmed that I received deprecation errors.

I applied this patch and ran those tests again and I no longer received those same deprecation errors.

👍

Mile23’s picture

Deprecation errors are the goal... :-) Just not in 8.6.x, AFAIC.

Wim Leers’s picture

Thanks!

Berdir’s picture

Makes sense in this case I guess, but I think deprecating something and immediately adding a @trigger_error() *if* all the usages in core are already updated is pretty standard now.

Contrib testing has suppress-deprecations enabled for a reason by default, because things *are* being deprecated in new versions.

I guess this case is a special as it was also deprecated in the stable version but in general, contrib has to live with deprecations for a while until you as a maintainer decide to only support the new version, which obviously can only happen after that version became stable.

Wim Leers’s picture

Contrib testing has suppress-deprecations enabled for a reason by default, because things *are* being deprecated in new versions.

The point of deprecating is to get contrib modules to fix those deprecations, so they stay current. Since core as of recently is providing security support for not one but two minors, that also implies contrib modules need to support two core minors.

Correct?

If correct, this puts contrib modules in a tough spot. @Berdir is right: this would solve the problem now, but will become a problem with the next core minor.

Berdir’s picture

Yes, but triggering deprecation messages != broken. That's why it's a deprecation and not just a straight removal/change.

Having no deprecations can not be goal or even requirement for contrib modules, not now anyway, not before we get to closer to 8.latest and we have to prepare contrib to be compatible with 9.x.

And anything test-related is excluded from BC anyway, that means core can break contrib tests as long as it doesn't break the actual functionality. Of course, it's hard to know that functionality isn't broken if tests aren't working, but those are the BC rules we have to work with somehow as contrib maintainers.

And yes, it's quite a lot of work and pain to keep up with that as contrib maintainer, but we all hope that we are trading in a constant amount of a bit of pain to a mountain of pain every few years when a new major comes out. Lets see if that plan holds up when 9.x is out ;)

Mile23’s picture

Just to add some more justification here... Like I said in #2996789: Deprecate Drupal\field\Tests\EntityReference\EntityReferenceTestTrait I filed it against 8.7.x.

Also, this specific trait doesn't have any need to be in one namespace or another, just that it's some i-dotting and t-crossing for the phpunit initiative. Not worth ruining contrib's day over.

See also crystal ball mode: #2607260-32: [meta] Core deprecation message introduction, contrib testing etc.

alexpott’s picture

Basically what @Berdir said.

I don't really see how this is going to help tests against 8.7.x are still going to fail. 8.6.x will have the new class and 8.5.x won't. So it still won't be possible to have a test in contrib that works across all supported branches and also prepare you for the future. The point being we're going to need that more flexible CI system that can define a matrix of test and set allowed failures etc... seems like we're building travis.

Wim Leers’s picture

Agreed.

This issue would "solve" the problem now but once we go to the next set of minors (from 8.5+8.6+8.7 to 8.6+8.7+8.8), we'll run into the exact same problem.

The only difference is that hopefully by that time we'll have improved the DrupalCI situation.

I'd love to get feedback from @Berdir and @alexpott on the proposed solution in #3001958: 4 test fails due to using deprecated code on 8.6 and 8.7 since #2996789: temporarily fork the test trait, which is basically: fork the test trait to get consistency across all branches, then stop the fork once the minimum supported core version is one where the undeprecated test trait is available.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

That said I don't care whether the deprecation is triggered in 8.7.x or 8.6.x as long as we trigger it somewhere. So let's commit this as it seems to help someone for now and maybe open a another issue to plan how to improve deprecation testing for contrib. Personally I don't think we should be recommend contrib uses deprecation testing until we have this ready. In this case the contributed project is only blocked because they chose to be early adopters.

Given that we'll have the problem regardless of whether we add deprecations only to the dev branch or the patch fix branch I do not think we should make a policy of only adding them to the dev branch.

Committed a97bb3aaf4 and pushed to 8.6.x. Thanks!

  • alexpott committed a97bb3a on 8.6.x
    Issue #3002011 by Mile23, Wim Leers, Berdir, gabesullice: Remove @...
alexpott’s picture

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.