Missing method visibility, bogus phpDoc and coding style in Cache backend classes

Nice!

These changes all seem fine to me, but probably best for catch to have a look since it touches the caching system.

I will simply leave the following thoughts as comments with little expectation that they might be applied to this patch. If a fuller review is requested, please let me know.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -38,8 +38,8 @@
@@ -60,7 +60,7 @@

@@ -60,7 +60,7 @@
    * @param $bin
    *   The cache bin for which the object is created.
    */
-  function __construct($bin);

I hope that I have made my point. Either there should be no type hinting or a complete application of type hinting. Somewhat is neither!!

Since type hinting is being added elsewhere in this patch, why not here?

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -60,7 +60,7 @@
    * Returns data from the persistent cache.
@@ -74,7 +74,7 @@ function __construct($bin);

@@ -74,7 +74,7 @@ function __construct($bin);
    * @return
    *   The cache or FALSE on failure.
    */
-  function get($cid);
+  public function get($cid);

Apparently type hinting is not required here either.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -111,7 +111,7 @@ function getMultiple(&$cids);
@@ -119,7 +119,7 @@ function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANENT, arra

@@ -119,7 +119,7 @@ function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANENT, arra
    * @param $cid
    *    The cache ID to delete.
    */
-  function delete($cid);
+  public function delete($cid);

Again no type hinting? Curious about what the standard is with regard to this patch.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -119,7 +119,7 @@ function set($cid, $data, $expire = CacheBackendInterface::CACHE_PERMANENT, arra
@@ -127,17 +127,17 @@ function delete($cid);

@@ -127,17 +127,17 @@ function delete($cid);
    * @param $cids
    *   An array of $cids to delete.
    */
-  function deleteMultiple(Array $cids);
+  public function deleteMultiple(Array $cids);

small nitpick: I believe our coding standards are for s/Array/array/. Please let me know if I am wrong.

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -148,12 +148,12 @@ function expire();
   /**
    * Checks if a cache bin is empty.
@@ -164,5 +164,5 @@ function garbageCollection();

@@ -164,5 +164,5 @@ function garbageCollection();
    * @return
    *   TRUE if the cache bin specified is empty.
    */
-  function isEmpty();
+  public function isEmpty();

Apparently no type hinting here is appropriate... *smiles*

You can't type hint a scalar in PHP.

@catch My comments were about the @param and @return directives.

Those lines are not being changed by the patch, so I've no idea why they would need to be typehinted.

Thanks for the comment @webchick.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -214,7 +212,7 @@ protected function validTags($checksum, array $tags) {
    *   Associative array of tags to flatten.
    *
-   * @return
+   * @return array
    *   Numeric array of flattened tag identifiers.

Is this not adding a type hint?

I'm reasonably convinced at this point that you're just trolling, but sure. I'll bite.

The difference is that:

+++ b/core/lib/Drupal/Core/Cache/CacheBackendInterface.phpundefined
@@ -38,8 +38,8 @@
@@ -60,7 +60,7 @@

@@ -60,7 +60,7 @@
    * @param $bin
    *   The cache bin for which the object is created.
    */
-  function __construct($bin);
+  public function __construct($bin);

Previous function had no type-hinting; adding it is scope creep.

+++ b/core/lib/Drupal/Core/Cache/DatabaseBackend.phpundefined
@@ -196,9 +194,9 @@ function garbageCollection() {
@@ -214,7 +212,7 @@ protected function validTags($checksum, array $tags) {

@@ -214,7 +212,7 @@ protected function validTags($checksum, array $tags) {
    * @param array $tags
    *   Associative array of tags to flatten.
    *
-   * @return
+   * @return array
    *   Numeric array of flattened tag identifiers.
    */

@param was already type-hinted, type-hinting @return brings them in-sync.

Thanks @webchick. I truly appreciate your response.

From a DX perspective, let me address what $bin in #8 is suppose to be. Is it an integer or a string? Based upon the current documentation, it is very unclear. Hence, my advocacy for type hinting whatever gets added or changed via a patch.

I am not trolling at all, and am annoyed that you would suggest such. I am simply advocating that type hinting gets added to whatever gets added to or touched via a patch.

You're right; that comment was out of line, and I apologize.

And yes, I generally agree with type-hinting when code is changed (though actually requiring on RTBC patches depends on the outcome of #1792992: Are typed @param / @return part of the documentation gate?), but no actual code here is changed. It's simply making the function scoping explicit, and is essentially a coding standards fix. Actually documenting the typehints on all of these functions requires deep inspection of each of these, in order to analyze whether $bin is indeed a string or an object or a $whatever, and that's not what's happening here. Separate concern, separate patch.

@webchick You have hit the nail on top of its head.

Yes, this patch does not change any code lines. As you say it 'is essentially a coding standards fix' and most often such patches get moved to @jhodgdon's queue.

However, if I might para-phrase @jhodgdon correctly, committing type hint addition patches are very low on her radar because of the time required to review the proposed type hint change(s).

My simple goal in #3 is that while we are addressing code lines, let's also make sure that the the type hinting of whatever has been adjusted is accurate.

As an aside, I am at a loss about why documentation patches cannot include minor code fixes and vis-a-versa.

Committed/pushed to 8.x, if there's more cleanup to do that can be done in a follow-up.