🇭🇺Hungary @Gábor Hojtsy

That's strange. Did you run the update status check at the top? What kind of issues does it report with 1.0.0?

That said, I love your experiments @zetagraph, don't feedback here as discouragement for the future, it is just not the right timing yet :)

I don't have capacity ATM to do this, but added a todo to where it needs work :)

Or it could go backwards until it finds one compatible release and suggest that instead of the last one. I guess that would still be better.

Right, it does say it is not compatible though, true?

Looks like our code only checks the very latest release and judges by that. It could be more elaborate and scan through the list of releases if there was one that is Drupal 11 compatible. Then it would need some kind of constraint resolution logic though as to what it suggests, etc.

See https://git.drupalcode.org/project/upgrade_status/-/blob/4.x/src/Project...

jjchinquist → credited Gábor Hojtsy → .

IMHO this sounds fantastic. For its use in the installer, but also in other cases, I think its important that it actually runs through, which is what may be hard to ensure. I guess you can already leave the page when a batch is running but now that it would be less evidently running, people may leave the page even more. That said, it would be fantastic if we would not block the user doing something just because we also need Drupal to do a lot of things in the background.

xjm → credited Gábor Hojtsy → .

Added module exists check too and tested that manually as well. Now if the module is already installed, it will be a warning, but if not, it will still be an error, making sure it is installed on Drupal 10.

Works fine in manual testing :) We don't have automated tests for DB requirements, that would be too heavy to cover.

Gábor Hojtsy → created an issue.

Gábor Hojtsy → created an issue.

Gábor Hojtsy → created an issue.

Gábor Hojtsy → created an issue.

bbrala → credited Gábor Hojtsy → .

Can/will tugboat maintain the same config file format / keys, etc. as long as the respective Drupal core versions are supported? I guess that is a key question if we put it into core, that is impossible to retroactively change :)

valthebald → credited Gábor Hojtsy → .

valthebald → credited Gábor Hojtsy → .

Created a branch in the issue fork for 3.x but it does not allow to update the fork on the gitlab UI, there are conflicts. I would love if someone could take it over, @TeeBeeCoder maybe?

https://git.drupalcode.org/project/l10n_server/-/blob/3.0.x/l10n_server/... is also limited to 50 yeah, so that needs work as well. I don't know if we need an update function that depends on how the D10 site is run.

Proposed an update function based on the above info. I don't have a way to test this, so this is as far as I could bring it.

Moving to major since it blocks parsing any translatable string on localize.drupal.org.

I assume the uri was capped at 50 to help limit index sizes. Not sure how allowing longer will change that, so maybe we should up to 100 or something more conservative, but I don't have enough deep database experience to tell how the performance would be affected either way.

The https://api.drupal.org/api/drupal/includes%21database%21database.inc/fun... docs confirm that, but also provide API functions to adding the key rather than altering directly as chatgpt said and also there is a built-in argument on db_change_field:

To maintain database portability, you have to explicitly recreate all indices and primary keys that are using the changed field.

That means that you have to drop all affected keys and indexes with db_drop_{primary_key,unique_key,index}() before calling db_change_field(). To recreate the keys and indices, pass the key definitions as the optional $keys_new argument directly to db_change_field().

[...]
You could use db_add_{primary_key,unique_key,index}() in all cases unless you are converting a field to be type serial. You can use the $keys_new argument in all cases.

I realized there are indexes on the URI field, two of them. So I asked chatgpt about whether I need to consider that too.

My prompt

There are indexes on the uri field, do I need to change anything about them when I change the field?

Chatgpt response

When you modify the length of a field that has indexes in Drupal 7, you must temporarily drop the indexes, modify the field, and then recreate the indexes. This ensures that the database schema updates correctly without running into issues related to the indexes.

Here's how you can update the uri field to 255 characters while handling the indexes properly:

1. Identify the indexes on the uri field. If you have a custom index, you'll need to note its name.
2. Update the .install file with the proper steps to drop the index, change the field, and recreate the index.

Here's an updated version of the function to handle indexes:

/**
 * Update the length of the 'uri' field in the 'l10n_server_project' table to 255 characters.
 */
function l10n_server_update_7001() {
  // Drop indexes on the 'uri' field before modifying the field.
  $indexes = db_index_exists('l10n_server_project', 'uri');
  if ($indexes) {
    db_query("ALTER TABLE {l10n_server_project} DROP INDEX uri_index");
  }

  // Define the new schema for the 'uri' field.
  $schema = array(
    'fields' => array(
      'uri' => array(
        'type' => 'varchar',
        'length' => 255,
        'not null' => TRUE,
        'default' => '',
      ),
    ),
  );

  // Update the 'uri' field to match the new schema.
  db_change_field('l10n_server_project', 'uri', 'uri', $schema['fields']['uri']);

  // Recreate the indexes on the 'uri' field.
  db_query("ALTER TABLE {l10n_server_project} ADD INDEX uri_index (uri)");

  // Indicate the update was successful.
  return t("Changed the length of the 'uri' field to 255 characters.");
}

Explanation:

• Dropping the index:
  • Before modifying the uri field, drop the index using db_query("ALTER TABLE {l10n_server_project} DROP INDEX uri_index");. Replace uri_index with the actual name of your index if it's different.
• Modifying the field:
  • The db_change_field function updates the field as described previously.
• Recreating the index:
  • After modifying the field, recreate the index using db_query("ALTER TABLE {l10n_server_project} ADD INDEX uri_index (uri)");. Again, replace uri_index with the correct index name.

Notes:

1. Ensure you replace uri_index with the actual name of the index on the uri field. If there are multiple indexes, you need to handle each one accordingly.
2. Backup your database before performing these updates to prevent data loss.

Run the database updates via /update.php or drush updatedb after adding this function to apply the changes.

Based on human review of https://api.drupal.org/api/drupal/includes%21database%21database.inc/fun... the proposed snippet looks like correct code.

Gábor Hojtsy → created an issue.

Got it in with this band-aid, and we can remove that later. It is a module in need of serious attention anyway.

Gábor Hojtsy → made their first commit to this issue’s fork.

Went a bit trigger happy with this :D Needed 🐛 Make sure 11.x branches of core are allowed Fixed too.

Gábor Hojtsy → created an issue.

This looks good IMHO, the module is super simple and I am not aware that the APIs it uses changed.

Updated the google doc for RC1, reviewed the known issues, etc. Definitely needs more work though.

This is now for RC1

Update subheaders to rc1 from beta1 in the issue summary.

@Spokje: yes I think so. 📌 Upgrade JavaScript dependencies to latest minor/patch releases Fixed has standard steps.

Tests pass on Drupal 10 and 11, but not on 9. That is not really on this issue to solve. That said, I can visibly see that the Drush command will not work with Drush 13, not even 12, because its not in the right place. So we need to fix that and also potentially add tests. https://git.drupalcode.org/project/upgrade_status/-/blob/4.x/tests/src/F... are Drush tests we use elsewhere and could be useful here.

Thanks all! I am not sure how @drumm updates the live site's potx, but once that is done, this will be online.

Looks good on a patch review, added test coverage looks great.

Update Drupal 11 RC date

Gábor Hojtsy → created an issue.

I made an entirely untested proposal for how this could look like. I think this would (if it works) report you the real problem that results in the neon file not being correctly generated.

In ✨ Disable ClassExtendsInternalClassRule Fixed we added the Drupal key by default to the template neon, but that template is further processed by the module to add the location of the included libraries. If the template is used without that, then the drupal key will be invalid. See DeprecationAnalyzer::createModifiedNeonFile(). Maybe something like that is happening here? If the includes cannot be added, an exception is thrown.

I noticed that the exception is only resulting in a user facing error on the UI (see top of UpgradeStatusForm::buildForm()). Unfortunately we are not surfacing environment exceptions to the Drush command which sounds like a problem :D See DeprecationAnalyzer::analyze() silently catching exceptions and UpgradeStatusCommands.php not calling initEnvironment but relying on the analyze, which silences problems.

Made an untested proposal, manual testing is welcome. Not sure how can we automatically test this. (We don't really have tests for the requirements checks unfortunately).

Thanks for working on this. I think the more we can reuse rather than copying it to our own would be better. Reviewing system_requirements(), there are various useful things it checks for that could affect upgrades, eg. module installed that is already not compatible with current version of core, missing module installed, entity updates missing, etc. These are beyond the database updates missing. So I think we may be better off with running the system requirements check and reporting if there were ANY errors from there. That would not be that specific about database updates, but it would help surface system issues.

Right you explained it yourself why this is not possible :) Added this to the project page now to make this clearer:

You must run compatibility checks on your current site against the next major version (ie. on a Drupal 10 site for Drupal 11 compatibility). It is not possible to run most compatibility checks on an upgraded site (ie. once you upgraded to Drupal 11), because the deprecated APIs, libraries, etc. are not present anymore.

Great, thanks!

Someone on twitter pointed out that the upgrade status job should be compared to the opt in test next major. Maybe that is useful for the docs too? See
https://www.hojtsy.hu/blog/2024-jul-05/continuous-forward-compatibility-... "How does this compare to running tests on the next major version?" section. @fjgarlin added on Slack:

We can clarify differences in the docs. It’s an individual job as opposed to a full variant and it can even help you with the fixes. Maybe this can be a comment in the open issue we have for docs improvements.

How do people end up with an empty composer.json at the wrong place? I believe this is the error message you all are hitting:

    throw new \Exception('The PHPStan binary was not found in the default vendor directory based on the location of ' . $composer_json_path . '. You may need to configure a vendor-dir in composer.json. See https://getcomposer.org/doc/06-config.md#vendor-dir. Attempted: ' . $binPath . '/phpstan.');

We can probably refine this text to say something like this?

The PHPStan binary was not found in the default vendor directory based on the location of ' . $composer_json_path . '. Is this the right composer.json file? If it is, you may need to configure a vendor-dir in composer.json....

I am not clear if it could be a valid case that we found a composer.json file that should not be deleted but is still not the right composer.json file. Could that happen? (I would avoid suggesting you remove useful composer.json files, if that were the case).

Gábor Hojtsy → changed the visibility of the branch 3444316-upgrade-status-d8 to hidden.

- Gitlab CI config should be updated to run the tests on the next major. That would prove that at least the tests still pass on Drupal 11.

- There is this change in the diff, does this still work on Drupal 8? What's the minimal version this still works on? We could potentially keep Drupal 8 support by making the code conditional?

diff --git a/potx.inc b/potx.inc
index 391275fbd664af69cd45bfdd70355221d183011a..56d1397bf3bbbd521491328e06a705db6a3274e1 100644
--- a/potx.inc
+++ b/potx.inc
@@ -573,7 +573,7 @@ function _potx_translation_export($translation_export_langcode, $string, $plural
     include_once 'includes/locale.inc';
   }
   else {
-    module_load_include('inc', 'potx', 'potx.locale');
+    \Drupal::moduleHandler()->loadInclude('potx', 'inc', 'potx.locale');
   }
 
   // Strip out slash escapes.

@Hetal.Solanki: @nilesh.addweb already assigned this to themselevs, are you two in contact?

These are top ones

[x] Drupal.Arrays.Array.CommaLastItem                                      44
[ ] Drupal.Files.LineLength.TooLong                                        30
[x] Generic.PHP.UpperCaseConstant.Found                                    17
[ ] Drupal.Commenting.FunctionComment.MissingReturnComment                 16
[x] Drupal.Commenting.InlineComment.SpacingAfter                           13
[ ] Drupal.Arrays.Array.LongLineDeclaration                                10

I think the ones marked [x] with a fixer would be low hanging fruit to fix and easy to review.

Gábor Hojtsy → created an issue.

Gábor Hojtsy → created an issue.

Fix typo in title for searchability.

Gábor Hojtsy → created an issue.

Sorry for not integrating this when it was done. Unfortunately definitely does not apply anymore. Also 3.x is not supported anymore.

Sorry for not integrating this when it was done. Unfortunately definitely does not apply anymore. Also 3.x is not supported anymore.

Drush commans now have some automated test coverage so that will not need to be manually tested.

Update title for correctness :)

Ok I made a suggestion for what needed changing. Marking it "ignore" will still make it show up in the reported list in Upgrade Status but it will be lower priority. Is that intended? (Similar to issues in future Drupal versions).

Yeah the "mistype" is the lack of method at the end of the strpos, the errors have the respective methods, while you match for RevisionableStorageInterface instead which does not appear as is.

Note that I was incorrect in February above. The generated config does indeed contain the Drupal key, we are putting it there :)

For the first pass of project browser recipe support, we planned to include basic input for recipes, eg. a contact form recipe would need your email address. A recipe working with a 3rd party service would need an API key. Configurability of recipe combinations beyond that (eg. this recipe suggests these other 3 recipes to install, you can choose) is a step beyond that but I think is somewhat of a continuation potentially in terms of interaction.

Interesting that the HTTP headers are included in the error message. Otherwise that looks like a normal progress AJAX response. The problem with the "missing .module file" has been resolved, you don't need an empty PHP file for PHPStan checking to pass anymore. That may be useful for people posting on this issue.

Ah right there is 📌 PHPStan does not return full results as of 1.11.5 if there was an "internal error" Fixed with the recent PHPStan, that means if there was broken PHP, none of the other errors are reported sadly.

Gábor Hojtsy → made their first commit to this issue’s fork.

Why core only? api.php files are supported for contrib too?

It appears you had a file locally that was named a PHP file but did not contain PHP. Look into the directory for your menu_markup extension and find this there. Let me know if that worked out.

Gábor Hojtsy → changed the visibility of the branch 3420355-upgrade-status-does to hidden.

You need to upgrade to Drupal 10.3 either way to upgrade to Drupal 11 later, so I don't think this is something I would consider a priority to solve. If we are to allow lower phpstan, that would require us to put in more conditional logic.

Span out the underlying PHPStan issue to 📌 PHPStan does not return full results as of 1.11.5 if there was an "internal error" Fixed , resolved that so we can continue here.

Gábor Hojtsy → created an issue.

So the underlying problem is that PHPStan does not return any files, it returns this error independent on files.

array ( 'totals' => array ( 'errors' => 1, 'file_errors' => 0, ), 'files' => array ( ), 'errors' => array ( 0 => 'Syntax error, unexpected T_STRING on line 5 while analysing file /var/www/html/web/modules/contrib/upgrade_status/tests/modules/contriby/fatal.php', ), )

Parsing this our code figures there were no files whatsoever and no PHPStan output gets into the scan. Question is what happened on the side of PHPStan recently and how we should parse the output now.

These errors are missing. I reproduced this manually too

    $file = reset($report['data']['files']);
    $message = $file['messages'][0];
    $this->assertEquals('fatal.php', basename(key($report['data']['files'])));
    $this->assertEquals("Syntax error, unexpected T_STRING on line 5", $message['message']);
    $this->assertEquals(5, $message['line']);
    $file = next($report['data']['files']);
    $this->assertEquals('UpgradeStatusTestErrorController.php', basename(key($report['data']['files'])));
    $message = $file['messages'][0];
    $this->assertEquals("Call to deprecated function upgrade_status_test_contrib_error_function_9_to_10(). Deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Use the replacement instead.", $message['message']);
    $this->assertEquals(13, $message['line']);
    $file = next($report['data']['files']);
    $this->assertEquals('ExtendingClass.php', basename(key($report['data']['files'])));
    $message = $file['messages'][0];
    $this->assertEquals("Class Drupal\upgrade_status_test_error\ExtendingClass extends deprecated class Drupal\upgrade_status_test_error\DeprecatedBaseClass. Deprecated in drupal:9.1.0 and is removed from drupal:10.0.0. Instead, use so and so. See https://www.drupal.org/project/upgrade_status.", $message['message']);
    $this->assertEquals(10, $message['line']);
    $file = next($report['data']['files']);
    $this->assertEquals('UpgradeStatusTestErrorEntity.php', basename(key($report['data']['files'])));
    $message = $file['messages'][0];
    $this->assertEquals("Configuration entity must define a `config_export` key. See https://www.drupal.org/node/2481909", $message['message']);
    $this->assertEquals(15, $message['line']);

In other words no result from phpstan.

So the fails are unrelated, but we need to resolve them somewhere anyway. Only these errors are found currently:

              <tr class="color-warning">
                      <td class="status-info">web/modules/contrib/upgrade_status/tests/modules/upgrade_status_test_error/upgrade_status_test_error.routing.yml</td>
                      <td>0</td>
                      <td>The _access_node_revision routing requirement is deprecated in drupal:9.3.0 and is removed from drupal:10.0.0. Use _entity_access instead. See <a href="https://www.drupal.org/node/3161210">https://www.drupal.org/node/3161210</a>.</td>
                  </tr>
              <tr class="color-warning">
                      <td class="status-info">web/modules/contrib/upgrade_status/tests/modules/upgrade_status_test_error/upgrade_status_test_error.css</td>
                      <td>0</td>
                      <td>The #drupal-off-canvas selector is deprecated in drupal:9.5.0 and is removed from drupal:10.0.0. See <a href="https://www.drupal.org/node/3305664">https://www.drupal.org/node/3305664</a>.</td>
                  </tr>
              <tr class="color-warning">
                      <td class="status-info">web/modules/contrib/upgrade_status/tests/modules/upgrade_status_test_error/config/views.view.remove_default_argument_skip_url.yml</td>
                      <td>109</td>
                      <td>Support from all Views contextual filter settings for the default_argument_skip_url setting is removed from drupal:11.0.0. No replacement is provided. See <a href="https://www.drupal.org/node/3382316">https://www.drupal.org/node/3382316</a>.</td>
                  </tr>
              <tr class="color-warning">
                      <td class="status-info">web/modules/contrib/upgrade_status/tests/modules/upgrade_status_test_error/upgrade_status_test_error.info.yml</td>
                      <td>1</td>
                      <td>Add core_version_requirement to designate which Drupal versions is the extension compatible with. See <a href="https://drupal.org/node/3070687">https://drupal.org/node/3070687</a>.</td>
                  </tr>
 

Gábor Hojtsy → created an issue.

Merged. Will make a beta2.

Gábor Hojtsy → made their first commit to this issue’s fork.

pameeela → credited Gábor Hojtsy → .