Duplicate code in mysql/schema.inc (createKeySql = createKeysSqlHelper)

Queued for testing

This might apply to 7.x, but it doesn't against 8.x, the files have been moved around a lot, everything is in core/ now and the classes are below core/lib/Drupal/Core/.. according to the PSR-0 standards.

Updated the patch to look at the Schema.php in core 8.x. Rerolls jonvk's suggested changes.

I've just realised that and rerolled to try and correct this.

Rerolled to try and correct earlier patch issue

According to the latest code from 8.x, this code appears to have been fixed in the mysql/Schema.php and the duplicate function removed. (We've been looking at this as part of Drupal Ladder this afternoon - hence trying to patch).

Sorry my fault for not looking carefully enough.

Hi,
sprinting at devdays in Dublin I took the liberty and claimed the work to be done here.
Pls review my patch. There are a bunch on unit tests for the schema class added as well.

Setting to needs review.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.phpundefined
@@ -293,17 +314,22 @@ protected function createKeySql($fields) {
+   *
+   * @deprecated
+   *   Duplicated function, use createKeysSql() instead.
+   *
+   * @see createKeysSql()
+   */
   protected function createKeysSqlHelper($fields) {
-    $return = array();
-    foreach ($fields as $field) {
-      if (is_array($field)) {
-        $return[] = '`' . $field[0] . '`(' . $field[1] . ')';
-      }
-      else {
-        $return[] = '`' . $field . '`';
-      }
-    }
-    return implode(', ', $return);
+    return $this->createKeySql($fields);

This is an internal, protected helper method. It's not part of an interface/API, so we can simply remove it. I think :)

I did a grep over the complete drupal dir. The reason why I did not remove it, was that there is potentionally the chance someone extended the class and uses this function - even if it is not the desired way to do.
Marking it deprecated and leaving it in for some time will give them the chance to migrate.

Actually I have no clue why the patch is failing .. says it fails to setup a drupal system. Can someone help?

#14: Duplicate_code_in_mysql_schema.inc_createKeySql=createKeysSqlHelper-1224892-14.patch queued for re-testing.

#14: Duplicate_code_in_mysql_schema.inc_createKeySql=createKeysSqlHelper-1224892-14.patch queued for re-testing.

typos in method call probably caused the tests to fail ... will see ;)

just to start the tests

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -293,17 +325,22 @@ protected function createKeySql($fields) {
   protected function createKeysSqlHelper($fields) {

If createKeysSqlHelper is not present in Drupal 7, I don't think we should keep the deprecated stub in Drupal 8. Otherwise, looks fine to me.

It's also in Drupal 7, so we might keep it for one more version.

Moving to the documentation component so Jennifer can have a look.

Oops, sorry, my bad. I didn't read down far enough. Good catch!

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -33,7 +33,10 @@ class Schema extends DatabaseSchema {
+   * @param string $table
+   * @param bool $add_prefix
...
   protected function getPrefixInfo($table = 'default', $add_prefix = TRUE) {

@@ -219,6 +228,11 @@ protected function processField($field) {
+   * @return array
...
   public function getFieldTypeMap() {

I'm not sure why these missing params / returns are being added here, but they're missing the descriptions of what they are, so those need to be added, too.

But it seems like this is crunching two issues into one; might be easier to split the docs improvements out (assuming #2057809: Correct the @param and @return statements on dbtng doesn't cover it already that is), and just fix the problem in the issue title here.

+++ b/core/lib/Drupal/Core/Database/Driver/mysql/Schema.php
@@ -293,17 +325,22 @@ protected function createKeySql($fields) {
+   * @deprecated
+   *   Duplicated function, use createKeysSql() instead.

I agree with mikl that I think this entire function can be removed, since the impact on module developers would be minimal.

Oops.

The issue #2057809 does not cover the Driver directory.
As for the proposal to split the doc improvments into a separate issue , I'd rather spend the time in an other issues than cherry picking the changes from the patch.
As for the removal of the function I have no doubt in removing this if you insist.

Going back to just fixing the original issue, without the additional docs changes, it looks like iaine's patch in #8 works.

Reloading it here to see. And adding "needs backport to 7" tag.

Aah, should be createKeySql, not createKeysSql, let's try that again.

+1 for removing duplicated code. As per #29, we were clear to remove the entire method, and #34 looks good. However, I'm not sure if this can be done post-beta (impact still seems minimal since this was a protected helper method).

This issue is a minor bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. There is no point maintaining 100% duplicated protected methods. Committed 8a8c28c and pushed to 8.0.x. Thanks!

Here's the D7 patch.

D7 patch in #39 looks good. The changes are essentially same as the D8 version but applied to a different file.
What are the implications of removing the protected function though?

Thanks, erik.erskine. Not sure of the implications of removing the protected function in D7.

Maybe time to close this issue??