Deprecate PhpStorage ::writeable() methods for removal in Drupal 11 with no replacement

Successfully applied patch #1. it looks good to me.

This seems to be a good idea.

public function writeable() {
return $this->writable();
}

For me it's RTBC +1.

There's a typo in 3138595-1.patch on the deprecation notice in PhpStorageInterface.php on comment #2. Patch to follow.

Hi,

Patch applied successfully #5

Looks good to me. Good to move it to RTBC.

Nice catch @walangitan

Thanks for working on this. More consistent spelling in APIs will mean fewer accidental bugs.

1. +++ b/core/lib/Drupal/Component/PhpStorage/FileReadOnlyStorage.php
   @@ -69,6 +69,13 @@ public function getFullPath($name) {
      public function writeable() {
   +    return $this->writable();
   +  }
   
   +++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
   @@ -148,6 +148,13 @@ public function getFullPath($name) {
      public function writeable() {
   +    return $this->writable();
   +  }
   

   The old implementations should also @trigger_error() the deprecation, and have a legacy test of the expected deprecation.

2. +++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
   @@ -55,9 +55,19 @@ public function save($name, $code);
   +   * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0.
   

   This should have an @see to a change record. It's also listing the wrong thing -- it should say PhpStorageInterface. Edit: But actually, see the next point.

3. +++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
   @@ -55,9 +55,19 @@ public function save($name, $code);
   +  /**
   +   * Whether this is a writable storage.
   +   *
   +   * @return bool
   +   */
   +  public function writable();
   

   Adding a new method to an interface is a BC break, because it changes the interface contract. We can add new methods to an interface if there's a 1:1 relationship with an abstract base class that implements the methods; then, the base class can provide the new method implementation. Reference: https://www.drupal.org/core/d8-bc-policy#interfaces

   Unfortunately, we don't seem to have that here.

   So, I think we need to probably drop the new method from the interface. We might be able to create an additional interface as a BC layer that gets merged into the current interface in D10. Edit: The deprecation messages / documentation would need to explain all that; otherwise we might not complete it correctly in D10.

Updated patch.

There are some "writeable" text and "is_writeable" function in the vendor folder.

Regarding #9:
Thanks for taking this forward, however the patch at #9 does not addresses the concerns pointed at #7.
Resetting for needs work.

I am unassigning this ticket as I have another ticket on my plate.

Thanks Kumar Kundan, #17 is a good start to address the BC concern, just few improvements over what is in #17:

1) Its better to implement the 'PhpStorageInterface' interface in the abstract class itself and have the abstract class only been implemented in the class actually defining the functions body.

2) deprecation message needs improvement for @trigger_error:
some thing like -
%thing% is deprecated in %in-version% and will be VERB ADVERB %removal-version%. %extra-info%. See %cr-link%
(make this consistent where ever used)

3) +++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php

+ * @deprecated in drupal:9.1.0 and is removed from drupal:10.0.0.
change with : + @deprecated in Drupal:9.0.0 and will be removed "from / before" Drupal:10.0.0.
(make this consistent where ever used)

4) +++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
"public function writeable();" is removed from the interface
and added the new function + public function writable();

writable function I see is placed in abstract class which is good however the original writeable() function must remain in the interface else it will break the BC.

The interface must not change at all.

Looks like a long patch to review... will share more findings if found :)

Regarding #21 patch looks good, just a small suggestion/change.

Interface PhpStorageInterface already implemented by abstract class PhpStorageBase. If you are extending the abstract class PhpStorageBase, then there's no need to implement the PhpStorageInterface again.

+++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
class FileStorage extends PhpStorageBase implements PhpStorageInterface must be class FileStorage extends PhpStorageBase

+++ b/core/lib/Drupal/Component/PhpStorage/FileReadOnlyStorage.php
class FileReadOnlyStorage extends PhpStorageBase implements PhpStorageInterface must be class FileReadOnlyStorage extends PhpStorageBase

Thanks Kumar Kundan for working on this, #26 looks almost ready just a small change:

Done changes as mentioned in #28, please review.

Regarding #30

Thanks for the patch but seems you removed spaces against all the lines within the class, it only is to be removed (only 2 spaces) from the line containing function signature.

I am working on it.

All the points of @xjm from comment #7 are still open.
There are still coding standard messages for the latest patch (see: https://www.drupal.org/pift-ci-job/1729166).
I think we should be using #2972224: Add .cspell.json to automate spellchecking in Drupal core, now that it has landed.

Correcting the coding standard errors from drupal CI.

Interdiff:

diff -u b/core/lib/Drupal/Component/PhpStorage/PhpStorageBase.php b/core/lib/Drupal/Component/PhpStorage/PhpStorageBase.php
--- b/core/lib/Drupal/Component/PhpStorage/PhpStorageBase.php
+++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageBase.php
@@ -15,2 +15,3 @@
   abstract public function writable();
+
 }
diff -u b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
--- b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
+++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
@@ -54,7 +54,11 @@
   /**
    * Whether this is a writeable storage.
    *
-   * @deprecated in Drupal:9.0.0 and will be removed from Drupal:10.0.0.
+   * @deprecated in Drupal:9.0.0 and will be removed from Drupal:10.0.0. This is
+   *   no longer used. Use \Drupal\Component\PhpStorage\PhpStorageBase::writable
+   *   instead.
+   *
+   * @see https://www.drupal.org/project/drupal/issues/3138595
    *
    * @return bool
    */

@himanshu_sindhwani

When you post an updated patch, please also attach an interdiff file. There are instructions here: Creating an interdiff. Personally, I recommend using the interdiff utility. I think there is less room for making mistakes.

Hi @daffie,
As per #7 comment, we covered all the possible points.
For
#7.1:- We used @trigger_error() for showing the deprecation message.
#7.2:- We used @see for referencing this issue.
#7.3:- We created a new abstract class & used for Backward compatibility.

Pls, let us know if we missed any point/cases.
Thanks

+++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageBase.php
@@ -0,0 +1,17 @@
+/**
+ * Replace writeable with writable.
+ */
+abstract class PhpStorageBase implements PhpStorageInterface {

I'm not sure about adding this class. I guess we're adding this because we don't want to change the interface to add writable(). But then we have to support this unnecessary base class til at least Drupal 11 and have to coordinate removing it. If we're going to do this we might as when add a new interface that adds the new method. In Drupal 10 we move the writable method to PhpStorageInterface and deprecate the bridging interface. That way everything stays on interfaces and we don't end up with unnecessary class hierarchy. I think the docs for the bridging interface should tell people to not typehint on it.

Another thing that is quite amusing is that the ->writeable() method is not used in any runtime. So is it even necessary? The last runtime usage was in #2497243: Replace Symfony container with a Drupal one, stored in cache and there are no searchable contrib usages... http://grep.xnddx.ru/search?text=-%3Ewriteable%28%29&filename=

Thanks to @alexpott, for reviewing this issue.

There are two points of view.
1. Either we follow the BC & abstract class PhpStorageBase implements PhpStorageInterface { convert the abstarct class to interface.
2. Or we directlly make changes in PhpStorageInterface interface.

Note:- I believe the 2nd point will be better.

@xjm & @alexpott please suggest the approach.

@Kumar Kundan note that #40 points out that another option is to deprecate the writeable() method and not replace it we have no use-case in core and nothing in contrib. For me that's the preferred way forward.

I would postpone this on #2898947: Change "writeable" to "writable" in documentation, some changes are out of scope here or in the scope of that one.

Marked #3155135: Change "writeable" to "writable" ( Replace method names ) as duplicate, please credit @rajandro and @matts213 if applicable.

And the blocker is in.

Per #42 if I understood it well. Deprecated writeable() directly, no replacement. But changing the method name in tests is fine, So changed.

+++ b/core/includes/install.core.inc
@@ -2185,7 +2185,7 @@ function install_check_requirements($install_state) {
-        $requirements["$file file writeable"] = [
+        $requirements["$file file writable"] = [

FYI, this is in the scope of #3155400: Change $requirements["$file file writeable"] to $requirements["$file file writable"], not here.

+++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
@@ -55,6 +55,11 @@ public function save($name, $code);
+   * @see https://www.drupal.org/node/2898947

Self-review, this should be the link of CR

CR added, Updating URL.

+++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
@@ -55,6 +55,11 @@ public function save($name, $code);
+   * @deprecated in drupal:9.0.0 and is removed from drupal:10.0.0. No replacement.

Another self-review, over 80 chars.

Addressing #50

This issue is being reviewed by the kind folks in Slack, #needs-review-queue-initiative. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge request as a guide.

@expectedDeprecation appears to no longer be used in Drupal core so tags for test to replace though.
Change record would have to be updated for deprecated in 10.1 and removed in 11

Thanks

Probably their own tests

Use the expectDeprecation() method instead. See https://www.drupal.org/node/3176667

Updated the @expectedDeprecation with expectDeprecation().

Verified the patch #60 on Drupal version 10.1.x. The patch applied cleanly and I have added the after patch screenshots of both files for reference.

That wasn't the correct fix. See change record

Thanks @jungle!

+++ b/core/lib/Drupal/Component/PhpStorage/FileReadOnlyStorage.php
@@ -69,6 +69,7 @@ public function getFullPath($name) {
+    @trigger_error('writeable() is deprecated in drupal:10.1.0 and will be removed from drupal:11.0.0. No replacement. See https://www.drupal.org/node/3155413', E_USER_DEPRECATED);

+++ b/core/lib/Drupal/Component/PhpStorage/FileStorage.php
@@ -148,6 +148,7 @@ public function getFullPath($name) {
+    @trigger_error('writeable() is deprecated in drupal:10.1.0 and will be removed from drupal:11.0.0. No replacement. See https://www.drupal.org/node/3155413', E_USER_DEPRECATED);

These deprecation messages would appear to refer to a global procedural writeable() function. However, they are actually about Drupal\Component\PhpStorage\PhpStorageInterface::writeable().

The change record also doesn't explain what the code should do instead if for some reason some code happens to use this method.

Plus, the current scope (of entirely deprecating the method without any replacement) seems much broader than just changing the spelling to not have the silent "e", so we should update it with how we came to this solution of just entirely removing the whole interface method.

Addressing @xjm's review. Added the new method ::writable() as the replacement of ::writeable().

Both IS and CR were updated.

IS and CR look better now.

Thanks, @smustgrave

+++ b/core/lib/Drupal/Component/PhpStorage/PhpStorageInterface.php
@@ -55,6 +55,20 @@ public function save($name, $code);
+  public function writable();

Note: writable() added to the interface is a BC break. Not sure if it's allowed.

The extra method is OK under the 1-1 exception for interface changes https://www.drupal.org/about/core/policies/core-change-policies/bc-policy

Committed 09a5ff9 and pushed to 10.1.x. Thanks!

#74 thanks!

BTW, just published the CR.

I think there was a misunderstanding here. In #68 and #69 I wasn't saying that the new approach (of deprecating the methods with no replacement) was wrong; I was just indicating that the IS and CR needed to be updated accordingly.

#40 and #42 make a case for deprecating the method with no replacement because as of those comments it was not used in core or contrib. That's fine, so long as we update the issue title, issue summary, and change record to justify this and to explain to someone who might have for whatever reason used this in custom code whatever it is they should do instead.

Discussed with @catch and we agreed on a revert. Thanks!

Just doing the title for now.

Sorry for the misunderstanding.

Corrected the title, it's ::writeable(), not ::isWriteable

IS updated, CR updated.

I missed that too. Think it should be good now though.

Committed to 10.1.x, thanks!