Remove or deprecate the class, remove its conditional usage in \Drupal\Core\Cache\ApcuBackendFactory::__construct().

CommentFileSizeAuthor
#29 interdiff_27-29.txt1.06 KBinit90
#29 3058498_29.patch2.73 KBinit90
#27 interdiff_24-27.txt1.18 KBinit90
#27 3058498-27.patch2.71 KBinit90
#24 interdiff-3058498-23-24.txt784 bytesyogeshmpawar
#24 3058498-24.patch2.79 KByogeshmpawar
#23 interdiff-3058498-21-23.txt1.32 KByogeshmpawar
#23 3058498-23.patch2.77 KByogeshmpawar
#21 interdiff_14-21.txt1.36 KBleolandotan
#21 core-deprecate-Apcu4Backend-3058498-21.patch2.9 KBleolandotan
#14 core-deprecate-Apcu4Backend-3058498-14.patch2.67 KBfconnolly
#10 core-deprecate-Apcu4Backend-3058498-10.patch8.44 KBfconnolly
#9 core-deprecate-Apcu4Backend-3058498-9.patch2.53 KBfconnolly
#8 core-deprecate-Apcu4Backend-3058498-8.patch1.64 KBfconnolly
#6 core-deprecate-Apcu4Backend-3058498-6.patch819 bytesfconnolly

Comments

Berdir created an issue. See original summary.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Issue tags: +Novice
fconnolly’s picture

fconnolly commented

I'm going to take a swing at this. Should the class be replaced by anything in ApcuBackendFactory? Or should an error be raised?

fconnolly’s picture

fconnolly commented
Assigned: Unassigned » fconnolly
cilefen’s picture

cilefen commented

@fconnolly: see https://www.drupal.org/core/deprecation

fconnolly’s picture

fconnolly commented
StatusFileSize
new core-deprecate-Apcu4Backend-3058498-6.patch819 bytes

I've uploaded a test patch. I believe I've deprecated the class the correct way, and that is the only change I've made. However, the project does not pass the tests on my local machine. I don't believe that just deprecating a function should break any tests? So I'm uploading for feedback/clarification. I also have not changed any of the actual code in Apcu4BackendFactory as I couldn't properly test any of my changes

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Active » Needs work

Not sure which tests you did run, but this would only be loaded on PHP 5, if tests failed that might have had other reasons.

The other thing you need to do is remove the code in ApcuBackendFactory that conditionally uses this class.

fconnolly’s picture

fconnolly commented
StatusFileSize
new core-deprecate-Apcu4Backend-3058498-8.patch1.64 KB

I've removed the conditional usage in ApcuBackendFactory. Also just to clarify, Apcu v4 would only be used if the user has PHP 5 or less, and Drupal 8.8.x no longer supports PHP 5, so it's safe to remove it?

Also, I tried looking in the composer.json file a little bit and didn't see anything about apcu v4, but do we need to including the apcu v4 extension somewhere in the source code?

fconnolly’s picture

fconnolly commented
StatusFileSize
new core-deprecate-Apcu4Backend-3058498-9.patch2.53 KB

Fixed a coding style error. Also updated the ApcuBackendTest class, didn't realize it tested the version separately instead of using the factory

fconnolly’s picture

fconnolly commented
StatusFileSize
new core-deprecate-Apcu4Backend-3058498-10.patch8.44 KB

Removed use statement from ApcuBackendtest

fconnolly’s picture

fconnolly commented
Status: Needs work » Needs review
fconnolly’s picture

fconnolly commented
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
diff --git a/core-deprecate-Apcu4Backend-3058498-6.patch b/core-deprecate-Apcu4Backend-3058498-6.patch
new file mode 100644

new file mode 100644
index 0000000000..7d66fd481b

index 0000000000..7d66fd481b
--- /dev/null

--- /dev/null
+++ b/core-deprecate-Apcu4Backend-3058498-6.patch

+++ b/core-deprecate-Apcu4Backend-3058498-6.patch
@@ -0,0 +1,21 @@

looks like you included the older patch files into the patch.

fconnolly’s picture

fconnolly commented
StatusFileSize
new core-deprecate-Apcu4Backend-3058498-14.patch2.67 KB

Ah, sorry, it's my first time using patches, I committed the older patch files in the issue branch. Fixed. Thanks for your patience

fconnolly’s picture

fconnolly commented
Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 14: core-deprecate-Apcu4Backend-3058498-14.patch, failed testing. View results

fconnolly’s picture

fconnolly commented
Status: Needs work » Needs review
berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs review » Needs work 
+++ b/core/lib/Drupal/Core/Cache/Apcu4Backend.php
@@ -2,10 +2,16 @@
 
+@trigger_error(__NAMESPACE__ . '\Apcu4Backend is deprecated in Drupal 8.8.x Apcu 4 was built for PHP 5 and
+ Apcu 5 is built for PHP 7+. Use ApcuBackend instead');
+
 /**
  * Stores cache items in the Alternative PHP Cache User Cache (APCu).
  *
  * This class is used with APCu versions >= 4.0.0 and < 5.0.0.
+ * @deprecated Apcu4Backend is deprecated in Drupal 8.8.x Apcu 4 was built for PHP 5 and
+ * Apcu 5 is built for PHP 7+. Use ApcuBackend instead
+ * @see ApcuBackend
  */
 class Apcu4Backend extends ApcuBackend {

The code changes themself look good, but we need to improve wording and spacing a bit here.

Id' keep the @trigger_error() on a single line, even if it is long.

The part inside the docblock with @deprecated must not be more than 80 characters however and needs a line break earlier.

We should also have an empty line above the @deprecated and above the @see. and the @see needs to use the full namespace.

I fear we also have pretty strict rules on having change records, even when there's really nothing else to say, so we need to create one with the add link on https://www.drupal.org/list-changes/drupal. And then link to that in the @see below and with See ...

I'm not sure if we need the part with "Apcu 4 was built..", I'd just leave that out. Suggestion: ...\Apcu4Backend is deprecated in drupal:8.8.0 and will be removed from drupal:9.0.0. Use ApcuBackend instead. See link-to-change-record"

Same text for @deprecated, except there the See link part is a separate @see.

leolandotan’s picture

leolandotan commented
Assigned: fconnolly » leolandotan

I'll try to work on this and create an initial change record as well.

leolandotan’s picture

leolandotan commented
Issue tags: +Needs change record

I added the "Needs change record" tag based on the recommendation of @Berdir.

leolandotan’s picture

leolandotan commented
Assigned: leolandotan » Unassigned
Status: Needs work » Needs review
StatusFileSize
new core-deprecate-Apcu4Backend-3058498-21.patch2.9 KB
new interdiff_14-21.txt1.36 KB

I have added the changes based on @Berdir's recommendations and tried to follow a deprecation pattern from other issues. I have also created a change record for your review.

Hope everything is in order.

Thanks!

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: Unassigned » yogeshmpawar
yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
StatusFileSize
new 3058498-23.patch2.77 KB
new interdiff-3058498-21-23.txt1.32 KB

I have updated the deprecation message as per the format & Agree with @berdir suggestion over to remove "Apcu 4 was built.." part. Added an interdiff as well.

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Assigned: yogeshmpawar » Unassigned
StatusFileSize
new 3058498-24.patch2.79 KB
new interdiff-3058498-23-24.txt784 bytes

Missed one thing in the previous patch so added back with an interdiff.

Status: Needs review » Needs work

The last submitted patch, 24: 3058498-24.patch, failed testing. View results

yogeshmpawar’s picture

yogeshmpawar
He/Him/His
Primary language English
Location Pune(MH), INDIA 🇮🇳
commented
Status: Needs work » Needs review

Above JS test fails are unrelated.

init90’s picture

init90
Location Ukraine
commented
StatusFileSize
new 3058498-27.patch2.71 KB
new interdiff_24-27.txt1.18 KB

I've corrected some moments in deprecation message. Also, I decided use deprecation message format suggested by @berdir in comment #18, because it seems more relevant for our case.

init90’s picture

init90
Location Ukraine
commented
Issue tags: -Needs change record

I've updated CR.

init90’s picture

init90
Location Ukraine
commented
StatusFileSize
new 3058498_29.patch2.73 KB
new interdiff_27-29.txt1.06 KB

I've changed deprecation format(one more time:), according to: Adopt consistent deprecation format for core and contrib deprecation messages

fconnolly’s picture

fconnolly commented
Status: Needs review » Reviewed & tested by the community

Looks good to me! :+1:

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed fa4c517 and pushed to 8.8.x. Thanks!

  • catch committed fa4c517 on 8.8.x 
    Issue #3058498 by fconnolly, yogeshmpawar, init90, leolando.tan, Berdir...
catch’s picture

catch
he/him
Primary language English
commented

Status: Fixed » Closed (fixed)

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