🇳🇮Nicaragua @dinarcon

Thanks for working on this @akulsaxena

Hi @jvbrian It is indeed strange to add the spaces to those files when such change is not necessary. The current status of the issue is Needs Review. Can you verify that the necessary changes, as instructed in the issue summary, have been implemented? And if so, can you add a comment indicating you have reviewed the issue and change the issue status to Reviewed & tested by the community (RTBC)?

I have rebased the MR after 📌 Fix cspell and phpcs issues Active The pipelines are already passing.

Thanks @baltowen @akulsaxena. All checks are passing. Merged.

Marking as fixed.

@chandansha, thank you for making the MR to resolve this issue. This issue is tagged for a Novice, which the special tags page → explains "would make a good project for someone who is new to the Drupal contribution process." Since you have over 30 contribution credits, including Drupal core ones, the community expects you to be leaving "Novice" issues to others new to Drupal contribution.

@ankitv18 you also have ~300 issue credits. Reviewing and testing issues is also valuable contribution. It would have been a good opportunity for some new to the community to participate.

Thank you both for your time. Because neither of you is relatively new to the community I am not assigning credit in this issue. You can question my decision. See What to do if you believe there is a mistake in issue crediting → for more information.

📌 Fix cspell and phpcs issues Active was opened as a follow up and is not tagged as Novice. You are invited to work on that issue as well as many other contribution opportunities → .

dinarcon → created an issue.

dinarcon → created an issue.

dinarcon → created an issue.

Thanks for updating the MR @rohan_singh. Good catch of the undefined variable @baltowen. I pushed a commit to have separate messages when the variables are NULL vs an empty array. If a user places the configuration in the wrong location, it would be better to be explicit about such configuration not being set.

Tested the two affected service methods / Drush commands. Things are working as expected.

For the new system, we can take a look at the https://www.drupal.org/project/migrate_upgrade → module. In addition to generating configuration entities like this module does, it also:

• Alters a subset of migrations. See \Drupal\migrate_upgrade\MigrateUpgradeDrushRunner::applyFilePath
• Applies a prefix to all migrations: See \Drupal\migrate_upgrade\MigrateUpgradeDrushRunner::modifyId
• Checks for existing migration IDs to avoid conflicts: See \Drupal\migrate_upgrade\MigrateUpgradeDrushRunner::substituteIds
• Takes into account migration derivers: See \Drupal\migrate_upgrade\MigrateUpgradeDrushRunner::expandPluginIds

For the sorting by weight logic, I suggest we leverage Core patterns: See \Drupal\Component\Utility\SortArray

Something like this in \Drupal\views\Plugin\views\filter\FilterPluginBase::buildGroupSubmit

uasort($group_items, ['Drupal\Component\Utility\SortArray', 'sortByWeightElement']);

There are more examples in \Drupal\Tests\Component\Utility\SortArrayTest

Regarding min_api_version and max_api_version properties, does the WXR file contains such information? If we queried the database, we can probably find the version of all plugins installed. But I don't think that information is available in the WXR export. At least I could not see plugin version data in this reference file with Yoast data provided by @hongpong. I guess we would have to provide a way for the end user to indicate which version of a plugin they are using. Even auto-detecting which plugins were used in the WordPress site out of the content of the WXR file might be challenging.

Another observation is that the current wizard would play nice to interactively select and configure plugins to use. What would be the approach to take for generating the migrations with Drush? A single interactive command? Individual commands for each migration?

Some of the comments might be out of scope for the specific issue. But the plugin system would be a foundational part of the module and the way it is designed/implemented will have an impact at how other features are implemented.

Thanks @rohan_singh for working on this. I reviewed the MR and requested some changes. The ?? NULL is not needed and can be removed. Back to needs work.

Forgot to change to NR.

Just noticed this is filed under the Base Recipe component. I made my change in drupal_cms_dashboard recipe which I guess would map to the Track: Dasboard component. I might be overthinking things, but wanted to make sure I applied the code change in the right place.

Second attempt at contributing to Drupal CMS... :-)

I was not expecting to use a config action for this. Some of the thought process I went through...

1. The menu link is content not configuration.
2. This link is provided by the Announcements module. The menu UI does not allow to change it. But it can be disabled.
3. Let's implement hook_menu_links_discovered_alter. But where?
4. Unless I am missing something, there is no custom modules in Drupal CMS.
5. drupal_cms_installer seems like a place where hooks are implemented. After looking at the code, drupal_cms_installer_uninstall_myself uninstalls the installer. :)
6. By disabling the menu link via the UI, I noticed there is a core.menu.static_menu_link_overrides.yml configuration file is created.
7. Tried to place the configuration file in recipes/drupal_cms_dashboard/config/core.menu.static_menu_link_overrides.yml, but that did not work.
8. Mostly by chance, I stumbled upon an example overriding a menu link in web/core/recipes/feedback_contact_form/recipe.yml
9. Added a simpleConfigUpdate to recipes/drupal_cms_dashboard/recipe.yml
10. Is... this... the right approach?

Developing for Drupal CMS is indeed different than doing it for Core. Looking forward to learn more and for more opportunities to contribute.

dinarcon → made their first commit to this issue’s fork.

Testing the waters of contributing to Drupal CMS... :-)

Some things that might to be updated:

• I went with compact for the new text format. Is this good?
• The example date is May 23, 2024. Shall we use the full textual representation (F) or the short, three letters one (M)?
• As for the day, shall we use leading zeroes (m) or not (n).

My expectation that providing drupal_cms_olivero/templates/announcements.html.twig would overwridde the template, but it did not. Turned on Twig debugging and saw this:

<!-- THEME DEBUG -->
<!-- THEME HOOK: 'announcements_feed' -->
<!-- BEGIN OUTPUT from 'core/modules/announcements_feed/templates/announcements-feed.html.twig' -->
  <nav class="announcements">
    <ul>
                    <li class="announcement announcement--standard">
          <div class="announcement__title">
            <a href="https://www.drupal.org/about/announcements/blog/drupalcon-singapore-2024-what-awaits-you-in-the-lion-city">DrupalCon Singapore 2024: What Awaits You in the Lion City</a>
            <div class="announcement__date">14 Oct 2024 - 11:49</div>
          </div>
        </li>
              <li class="announcement announcement--standard">
          <div class="announcement__title">
            <a href="https://www.drupal.org/blog/drupal-11-released">Drupal 11 released</a>
            <div class="announcement__date">1 Aug 2024 - 17:57</div>
          </div>
        </li>
          </ul>
  </nav>

      <p class="announcements--view-all">
      <a target="_blank" href="https://www.drupal.org/about/announcements">View all announcements</a>
    </p>
  
<!-- END OUTPUT from 'core/modules/announcements_feed/templates/announcements-feed.html.twig' -->

Looking at core/modules/announcements_feed/templates/announcements-feed.html.twig I see:

{% include '@announcements_feed/announcements.html.twig' %}

Tried to overwrite announcements-feed.html.twig, copying into drupal_cms_olivero/templates/ and editing the file, but the core template was still being used.

dinarcon → made their first commit to this issue’s fork.

Is this a duplicated of 📌 Switch the body field back (again) to text_long Active ? I see this is filed under the Track: Blog component while the other issue is under Base Recipe. Not sure if that would mean making the change on different projects, hence the need for multiple issues.

dinarcon → created an issue.

The problem with duplicated migrations, due to using the same prefix, should also be solved with the fix from 🐛 Duplicative generated migration group causes uncaught error at review stage EntityStorageException Needs work

I tested this and I am not allow to submit the form is the prefix is already in used. We can probably improve the error message, but otherwise the name conflict is being detected.

Tested the latest code and was getting a fatal error when starting the wizard at /admin/structure/migrate/wordpress_migrate

The website encountered an unexpected error. Try again later.

ArgumentCountError: Too few arguments to function Drupal\wordpress_migrate_ui\Form\SourceSelectForm::__construct(), 0 passed in /var/www/html/web/core/lib/Drupal/Core/Form/FormBase.php on line 82 and exactly 1 expected in Drupal\wordpress_migrate_ui\Form\SourceSelectForm->__construct() (line 25 of modules/contrib/wordpress_migrate/wordpress_migrate_ui/src/Form/SourceSelectForm.php). 

I pushed a new commit passing the missing parameter.

The last pipeline was failing. It looks like during a merge @baltowen's phpcs fix was lost. I cherry-picked c636be41 and pushed. This should make phpcs pass.

dinarcon → made their first commit to this issue’s fork.

dinarcon → created an issue.

Ok, got my commits and branches a bit messed up, but I think I finally sorted things out...

I tested @baltowen commit (after rebasing the branch against 8.x-3.x) and the error is gone. For this, I used the wp-import-5-loremipsum-2024-11-25.xml file in https://gitlab.com/HongPong/wordpress-test-imports While MR #30 technically fixes the issue, I think we should go with a slightly different approach.

MR #30 will allow overwriting users based on their usernames. That could lead to account take over or at regain access that had been revoked.

The wordpress_authors.yml migration uses the email based on the XWR file. If a matching username is found, the email will be changed and someone else can get access to the site via a password reset link. Also, the migration sets the status to 1 always which means the user is active. A user that had been blocked would have their account reactivated if that username is present in the XWR file.

I propose we do not allow overwriting existing accounts. In MR #31 I expand on @baltowen's commit to add a safety check to prevent user accounts from being overwritten.

By the way, I included a message to indicating that is is not allowed to overwrite user accounts. For some reasons, those messages are lost when running the migration from the UI. When doing it from Drush, the messages are preserved. This is unrelated to the issue being discussed here. Still I wanted to point it out in case someone looks for the messages and does not find them.

dinarcon → made their first commit to this issue’s fork.

I think this error is coming from \Drupal\migrate\Plugin\migrate\source\SqlBase::setUpDatabase. There is a test case in \Drupal\Tests\migrate_drupal\Unit\source\DrupalSqlBaseTest::testSourceDatabaseError if people want to research further.

I do not think the error is related to wordpress_migrate. See related core issue.

Thanks @baltowen and @gaurav_manerkar for working on this! After implementing the suggestion to use --all as the option name, I reviewed and updated other function names and parameters. I also updated the Drush command names and included shorter aliases.

Great work @gaurav_manerkar! A few things I would like to include before merging.

• Rename the ignore-auto-increment-column option of the getAllTableAutoIncrementValues command to unfiltered.
• No need to provide validation for the entity group/type in getEntityList.
• Change group in getEntityList from argument to option.

Once the changes are implemented, let's update the README.md accordingly.

The https://www.drupal.org/project/wordpress_migrate_sql → module provides source and process plugins to extract data from a WordPress database. Collaboration between wordpress_migrate and wordpress_migrate_sql would be nice.

Took a step back. The source of the issue is having incomplete definitions for process plugins that leverage migration_lookup. An alternative solution to providing a placeholder for the missing migration configuration is to extract those process plugins out of the base migrations, as there are multiple examples already. MR 25 follows this approach. I also cherry-picked @baltowen's fix to default_author error.

Looking forward to feedback on both MRs. If we decide to go with #25, the topic of not creating base migration instances might be discussed in a follow up.

I had another look at the module to see how using derivatives would work. I think that should be addressed in a separate issue. For example, we could one "base migration" for all taxonomy migrations and that seems out of scope for this issue.

While investigating, I noticed the module is creating instances out of the "base migrations" shipped with the module. That seems unnecessary. Getting the plugin definition would suffice to accomplish what the \Drupal\wordpress_migrate\WordPressMigrationGenerator::createEntityFromPlugin method is expected to do. I guess this could be considered out of scope for this issue as well.

@hongpong what do you think about these remarks? And how would you like to proceed with this issue?

When using the migration_lookup process plugin, it is assumed that the migration configuration key is set. That defines the list of migrations used for the lookup operation. In migrations/wordpress_comment.yml there are currently two usages of migration_lookup that do not have the migration configuration defined (because they will be generated dynamically).

process:
  entity_id:
    plugin: migration_lookup
    source: post_id
    # migration generated dynamically
  pid:
    plugin: migration_lookup
    source: comment_parent
    # migration ID generated dynamically

In WordPressMigrationGenerator::createEntityFromPlugin there is a call to $plugin_manager->createInstance($plugin_id). In \Drupal\migrate\Plugin\MigrationPluginManager::createInstances there is a call to \Drupal\migrate\Plugin\Migration::getMigrationDependencies where the system tries to infer the required/optional dependencies for the migration plugin being created. Eventually \Drupal\migrate\Plugin\Migration::findMigrationDependencies is called when attempting to set a list of optional dependencies. In there, the each process plugins is analyzed. If usages of migration_lookup are found, the corresponding migration configuration key is added to the list of dependencies. When not set, the warning described in this issue is triggered.

  protected function findMigrationDependencies($process) {
	...
        if (in_array($plugin_configuration['plugin'], ['migration', 'migration_lookup'], TRUE)) {
          $return = array_merge($return, (array) $plugin_configuration['migration']);
        }
	...
  }

Back in \Drupal\wordpress_migrate\WordPressMigrationGenerator::createEntityFromPlugin there is also an explicit call to $migration_plugin->getMigrationDependencies(); which makes the warning to be triggered again.

Later on when processing the generated comment migration plugin, the missing migration keys are injected into the corresponding migration_lookup process definitions. Also, the migration dependencies are overridden.

        $process                                     = $migration->get('process');
        $process['entity_id'][0]['migration']        = $content_id;
        $process['comment_type'][0]['default_value'] = $storage->getSetting('comment_type');
        $process['pid'][0]['migration']              = $id;
        $process['field_name'][0]['default_value']   = $field_name;
        $migration->set('process', $process);
        $migration->set('migration_dependencies', ['required' => [$content_id]]);

At the end generating the configuration entities, the migration plugins have valid process sections. But at that point it is too late. The warnings have been triggered.

The current MR proposes to add the missing migration configuration in migrations/wordpress_comment.yml and set it to a _PLACEHOLDER_ which is later replaced by the proper value.

dinarcon → made their first commit to this issue’s fork.

smustgrave → credited dinarcon → .

✨ Get the current AUTO_INCREMENT value for a table Active has landed. Let's revisit this issue.

Thanks @baltowen and @gaurav_manerkar for working on this. Updating issue summary to reflect the two new methods added to the service.

Thanks for working on this @gaurav_manerkar

Drush 11 and prior required dependency injection via a drush.services.yml file. This approach is deprecated in Drush 12+ and removed in Drush 13. See https://www.drush.org/13.x/dependency-injection/ While Drush 11 could work with Drupal 10, that version has been EOL for about a year. Let's not use services files. See https://www.drush.org/13.x/commands/ for the recommended way to author Drush commands.

Also, a few new methods are being worked on. Once the following issues land, let's add Drush commands for the methods introduced in them:

• ✨ Get a list of all tables Active
• ✨ Get the current AUTO_INCREMENT value for a table Active
• ✨ Provide a way to to alter the AUTO_INCREMENT for a single entity Active

Pushed an update to getTableAutoIncrementValue method and a new getAllTableAutoIncrementValues method.

Please review.

Thanks for working on this @gaurav_manerkar I have rebased the MR to resolve conflicts introduced in recent commits.

I tested the MR and it works, but let's make make some improvements. I don't think we need to worry about adding prefixes or not to tables. Now that ✨ Get a list of all tables Active has landed, people can use that method to check for tables as they exist, prefix included when present.

Also, while not explicitly stated in the issue summary, the query you added to fetch the AUTO_INCREMENT can be adjusted to get the values for all tables. Let's include that in this issue.

I will take a first shot at this. Assigning to myself.

Marking as fixed.

Reviewed and tested. Thanks @baltowen

Reviewed and tested. Working as expected. Thanks @gaurav_manerkar and @baltowen

dinarcon → created an issue.

dinarcon → created an issue.

dinarcon → created an issue.

dinarcon → created an issue.

dinarcon → created an issue.

I think the original question is related to where the settings used by the module should be placed. That would be sites/default/settings.php or another file included there. The module provided migrate_skip_fields.settings.php file is meant to serve only as a reference of how to set the values, but the settings themselves would live in the standard location.

@scrypter you can instruct sites/default/settings.php to read migrate_skip_fields.settings.php from a custom module, but that would not be standard. Alternatively, you can copy the file into the sites/default/ folder and include it by adding this to sites/default/settings.php:

if (file_exists($app_root . '/' . $site_path . '/migrate_skip_fields.settings.php')) {
  include $app_root . '/' . $site_path . '/migrate_skip_fields.settings.php';
}

The module does not use the configuration management API nor makes use of configuration scheme.

@zartab farooquee I am not clear what you are suggesting nor how the code is related the module's functionality.

Updating status. Please change back to active and add more context if I have not addressed the questions.

Hi, I have released a new module to help with setting the AUTO_INCREMENT value for tables when using the MySQL driver https://www.drupal.org/project/auto_increment_alter → An article talking about this topic will be available at https://udrupal.com/auto-increment-alter-documentation Comments and feedback are very much appreciated.

Thanks! This has been released as part of version 1.0.0-alpha6 →

Tested the changes manually and things works as expected. The migrate_skip_fields_source_version is no longer required and defaults to '7'. Thanks everyone for your contributions. I will tag a new release with this fix.

Thanks for the report and the fix @_gradient_ and @ananya.k

I would say that most upgrades nowadays are from Drupal 7. Let's make this setting optional and provide a default to '7'.

xjm → credited dinarcon → .

benjifisher → credited dinarcon → .

benjifisher → credited dinarcon → .

Thanks for the patch @DamienMcKenna

Looking at it covers date formats (i.e. l, F j, Y - H:i), but no date types. (e.g. Long / date_format_long). I think date formats and date types are not two separate configuration entities in Drupal 11. The date type can be mapped to the id/label of the DateFormat configuration entity, and the date format mapped to the pattern. This would require querying the date_format_type and date_formats respectively. Thoughts?

benjifisher → credited dinarcon → .

Fixed issue title.

dinarcon → created an issue.

Thanks for working on this. I tested the MR and I got a never ending migration, processing the same records over and over. I think that is caused because reading the highwater value to add the condition to the query only happens in the first run of the batch inside an if ($this->batch == 0) condition.

The very first time that I run the migration, the highwater has not been set. So, the code from the MR keeps returning a query with LIMIT [BATCH_SIZE] OFFSET 0. Subsequent calls initializeIterator already have $this->batch greater than 0 so the code to add the highwater is never reached.

If I force the migration to stop and run it again, the highwater condition is set, but it is never updated in subsequent calls initializeIterator because of the same reason.

For this to work, I think the code that sets the highwater condition needs to be moved outside the if ($this->batch == 0) condition.

Thanks for the MR. As of the latest release, the content MR is no longer accurate. Some notes:

• The dependency on the migrate_upgrade module has been removed.
• A new required migrate_skip_fields_source_version setting has been introduced.
• The example of Radioactivity not offering an upgrade path is technically accurate, but the reference issue is about upgrading from Drupal 6 to Drupal 7. Can you file an issue for upgrading from Drupal 6/7 to Drupal 10/11? Then, we can link to the new issue. Or we could find a different module to use as example.

Setting back to needs work.

You need to use the machine name of the Drupal 7 entity type and bundle. All entities should be supported. Try:

$settings['migrate_skip_fields_by_name'] = [
  'user:user:field_name',
  'og_membership:*:*'
];

It is not feasible to include all entity types in the example settings. If the above does not work, add a new comment with your the value for migrate_skip_fields_by_name setting. Note that as of the latest release, a new required migrate_skip_fields_source_version should be used.

Fixed and included in 1.0.0-alpha5 release.

Updating status.

Fixed and included in 1.0.0-alpha5 release.

Updating status.

Fixed and included in 1.0.0-alpha5 release.

Fixed and included in 1.0.0-alpha5 release.

Fixed and included in 1.0.0-alpha5 release.

dinarcon → made their first commit to this issue’s fork.

dinarcon → created an issue.

dinarcon → created an issue.

dinarcon → created an issue.

dinarcon → created an issue.

Fixed and included in 1.0.0-alpha4 release.

Thanks to all mentoring coordinators for their impactful contributions! As for me, it is time to step down. I make mentoring activities a priority in the events I attend. But I have not been able to assist much with advance planning/coordination. I am still around and look forward to continue contributing to mentoring efforts to the extent possible.

benjifisher → credited dinarcon → .

mikelutz → credited dinarcon → .

dinarcon → created an issue.

Committed.

dinarcon → created an issue.

Thanks for the quick reply. Committing directly commit to 7.2.x is fine. I am not sure what happened with the issue fork. When I created my PR it was against the 7.x-2.x branch. I am surprised that it had a merge conflict because my commit changed only one line compared to HEAD. The MR now says that it is against 4.x with a lot more changes. I don't really understand what happened there.

BTW, sorry for changing the visibility of the `3450182-projects-moved-into` branch. I was trying to review things and clicked the wrong link. I changed it back to visible.

dinarcon → changed the visibility of the branch 3450182-projects-moved-into to active.

dinarcon → changed the visibility of the branch 3450182-projects-moved-into to hidden.

Added a MR with the suggested change.

dinarcon → created an issue.

ricardoamaro → credited dinarcon → .

This is still an issue. Version 4.0.3 contains core_version_requirement: ^9 || ^10 in the radioactivity.info.yml file, but the issue reported here is not being able to add the module via Composer. That is because 4.0.3 does not declare Drupal 10 compatibility in the composer.json file. See https://git.drupalcode.org/project/radioactivity/-/blob/4.0.3/composer.j...

This has already been fixed in https://git.drupalcode.org/project/radioactivity/-/commit/5b0200bc08ca66..., but that commit was not part of the 4.0.3 release.

Doing what is described in #6 or #7 allows the module to be installed. Tagging a new release would be very much appreciated.

dinarcon → made their first commit to this issue’s fork.

Thanks for stepping up @RobLoach I have added you as a maintainer of this project https://www.drupal.org/project/ckeditor_a11ychecker → and also of https://www.drupal.org/project/ckeditor_balloonpanel → as it is a dependency.

benjifisher → credited dinarcon → .

benjifisher → credited dinarcon → .

volkswagenchick → credited dinarcon → .

volkswagenchick → credited dinarcon → .

benjifisher → credited dinarcon → .

benjifisher → credited dinarcon → .

We might need the to review the skip logic in the view_mode and bundle process pipelines. Let's see what the test bot reports about the patch first.

Please review.

dinarcon → created an issue.