Installation fails on Drupal 8.0.0-beta6, using the latest xmlsitemap code from the 8.x-1.x branch. To reproduce the issue using drush, start with a newly-downloaded drupal-8.0.0-beta6.

drush si standard
drush en xmlsitemap

After attempting to enable xmlsitemap, I saw the following output.

Error: Class Drupal\xmlsitemap\Form\XmlSitemapSettingsForm contains 1 abstract method and must therefore be declared abstract or
implement the remaining methods (Drupal\Core\Form\ConfigFormBase::getEditableConfigNames) in
/home/vagrant/docroot/modules/contrib/xmlsitemap/src/Form/XmlSitemapSettingsForm.php, line 262

With config_translation enabled, attempting to enable xmlsitemap, I saw the following output.

$ drush en xmlsitemap
The following extensions will be enabled: xmlsitemap
Do you really want to continue? (y/n): y
exception 'InvalidArgumentException' with message 'Link templates accepts paths, which have to start with a leading slash.' in[error]
/home/vagrant/docroot/core/lib/Drupal/Core/Entity/EntityType.php:551
Stack trace:
#0 /home/vagrant/docroot/core/modules/config_translation/config_translation.module(95):
Drupal\Core\Entity\EntityType->setLinkTemplate('config-translat...', 'xmlsitemap.admi...')
#1 /home/vagrant/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(494): config_translation_entity_type_alter(Array,
NULL, NULL)
#2 /home/vagrant/docroot/core/lib/Drupal/Core/Entity/EntityManager.php(240):
Drupal\Core\Extension\ModuleHandler->alter('entity_type', Array)
#3 /home/vagrant/docroot/core/lib/Drupal/Core/Plugin/DefaultPluginManager.php(151):
Drupal\Core\Entity\EntityManager->findDefinitions()
#4 /home/vagrant/docroot/core/lib/Drupal/Core/Extension/ModuleInstaller.php(240):
Drupal\Core\Plugin\DefaultPluginManager->getDefinitions()
#5 /usr/share/php/drush/commands/core/drupal/environment.inc(130): Drupal\Core\Extension\ModuleInstaller->install(Array, true)
#6 /usr/share/php/drush/commands/core/drupal/environment.inc(197): drush_module_install(Array)
#7 /usr/share/php/drush/commands/pm/pm.drush.inc(1120): drush_module_enable(Array)
#8 [internal function]: drush_pm_enable('xmlsitemap')
#9 /usr/share/php/drush/includes/command.inc(359): call_user_func_array('drush_pm_enable', Array)
#10 /usr/share/php/drush/includes/command.inc(210): _drush_invoke_hooks(Array, Array)
#11 [internal function]: drush_command('xmlsitemap')
#12 /usr/share/php/drush/includes/command.inc(178): call_user_func_array('drush_command', Array)
#13 /usr/share/php/drush/lib/Drush/Boot/DrupalBoot.php(46): drush_dispatch(Array)
#14 /usr/share/php/drush/drush.php(76): Drush\Boot\DrupalBoot->bootstrap_and_dispatch()
#15 /usr/share/php/drush/drush.php(16): drush_main()
#16 {main}
CommentFileSizeAuthor
#21 xmlsitemap-fix-install-uninstall-2427509-21.patch11.8 KBMichelle
#15 interdiff12-15.txt729 bytesandrei.dincu
#15 xmlsitemap-fix-install-uninstall-2427509-15.patch11.74 KBandrei.dincu
#12 xmlsitemap-fix-install-uninstall-2427509-12.patch11.07 KBMichelle
#11 xmlsitemap-fix-uninstall-2427509-11.patch1.08 KBMichelle
#8 interdiff-2427509-6-8.txt3.56 KBpaulmckibben
#8 xmlsitemap-install-fails-2427509-8.patch10.12 KBpaulmckibben
#6 interdiff-2427509-5-6.txt2.01 KBpaulmckibben
#6 xmlsitemap-install-fails-2427509-6.patch6.26 KBpaulmckibben
#5 xmlsitemap-install-fails-2427509-5.patch4.06 KBpaulmckibben
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented

I found part of the problem. This change record requires link annotations to be updated:
https://www.drupal.org/node/2382937

However, after updating the link annotations in src/Entity/XmlSitemap.php as follows, I get a new error upon installation.

Updated code:

/**
 * Defines the XmlSitemap entity.
 *
 * @ConfigEntityType(
 *   (... unchanged code omitted ...)
 *   links = {
 *     "edit-form" = "/admin/config/search/xmlsitemap/{xmlsitemap}/edit",
 *     "delete-form" = "/admin/config/search/xmlsitemap/{xmlsitemap}/delete"
 *   }
 * )
 */

New error when attempting to install:
Error: Call to a member function getPath() on a non-object in
/home/vagrant/docroot/core/modules/config_translation/src/ConfigNamesMapper.php, line 238

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented

On further debugging, I see that ConfigNamesMapper->getBaseRoute() returns null (ConfigNamesMapper.php line 196). It is trying to get a route for "entity.xmlsitemap.edit_form". There is no such route in xmlsitemap.routing.yml.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented

It looks like this change record also applies: https://www.drupal.org/node/2306387 - "Entity HTML route patterns standardized"

I think this means the entity-specific route names in xmlsitemap.routing.yml need to be updated to correspond to the standard.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented
Title: Installation fails when config_translation is enabled » Installation fails on Drupal 8 beta 6
Priority: Normal » Major
Issue summary: View changes

Original issue was an installation failure when config_translation is enabled. However, this module fails to install at all, even without config_translation.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented
Status: Active » Needs review
FileSize
xmlsitemap-install-fails-2427509-5.patch4.06 KB

Please review the attached patch.

A total of 3 change records affected xmlsitemap, and this patch addresses all 3.
https://www.drupal.org/node/2382937 - the patch updates link annotations accordingly.
https://www.drupal.org/node/2306387 - the patch updates entity route names accordingly.
https://www.drupal.org/node/2407153 - the patch updates implementations of ConfigFormBase to implement the new method, Drupal\Core\Form\ConfigFormBaseTrait::getEditableConfigNames().

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented
FileSize
xmlsitemap-install-fails-2427509-6.patch6.26 KB
interdiff-2427509-5-6.txt2.01 KB

New patch attached. Some of the routes needed to be updated elsewhere in order for the admin screens to work.

penyaskito’s picture

penyaskito
he/him
Primary language Spanish
Location 🧑🏽‍🌾Seville 💃, Andalusia 🇪🇸, UTC+2 🇪🇺
CreditAttribution: penyaskito commented
Status: Needs review » Needs work

Config forms need to get editable configs from the factory, see change record at https://www.drupal.org/node/2407153.
Thanks for working on this, Paul!

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented
Status: Needs work » Needs review
FileSize
xmlsitemap-install-fails-2427509-8.patch10.12 KB
interdiff-2427509-6-8.txt3.56 KB

penyaskito, thanks for the review!

Here's a new patch and new interdiff. I believe I have found and updated all places that need to use the factory to get editable configs. If there are any I missed, please let me know the file and line number.

I also fixed a bug that was attempting to compare a URL object to a string. XmlSitemapGenerator.php, line 244, now uses the toString() method on the URL object so a valid comparison is possible on line 249.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
CreditAttribution: lauriii commented
Status: Needs review » Needs work

The latest patch didnt fix installing

Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle commented
Status: Needs work » Reviewed & tested by the community

The patch in #8 applies cleanly against the git checkout of the 8.x-1.x branch. I was able to enable the module with no errors on Drupal 8 Beta 7. /admin/config/search/xmlsitemap comes up with no errors as do all the tabs on it.

Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle commented
Status: Reviewed & tested by the community » Needs work
FileSize
xmlsitemap-fix-uninstall-2427509-11.patch1.08 KB

I ran into errors with uninstalling the module and found the lines that needed fixing and created a patch. However, when I tried to test it without your patch already being there, I ran into more errors on uninstall that you had already fixed. I'm thinking that my patch should be added to your patch to fix both install and uninstall. I wanted to put it up separately, first, though, so you can look at my changes. If you agree, I can merge these into one patch.

This is the relevant change record I went by, which I see you mentioned in #5. Wish I'd read all the comments on this issue. Would have saved me time. LOL! https://www.drupal.org/node/2407153

Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle at Mediacurrent commented
Status: Needs work » Needs review
FileSize
xmlsitemap-fix-install-uninstall-2427509-12.patch11.07 KB

I just realized I might as well combine them and send them for testing right away so it's ready to go if you have no objections. :) Here is our combined patch.

Status: Needs review » Needs work

The last submitted patch, 12: xmlsitemap-fix-install-uninstall-2427509-12.patch, failed testing.

Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle at Mediacurrent commented

I'm looking at the test results and the error is reporting is on this line in XmlSiteMapTestBase.php:

$this->error($this->l($message, $message_url)), 'User notice');

So is it our patch is failing the tests or the tests are failing our patch? :)

andrei.dincu’s picture

andrei.dincu CreditAttribution: andrei.dincu commented
FileSize
xmlsitemap-fix-install-uninstall-2427509-15.patch11.74 KB
interdiff12-15.txt729 bytes

New patch.
Interdiff between patch from comment 12 and this one.

andrei.dincu’s picture

andrei.dincu CreditAttribution: andrei.dincu commented
Status: Needs work » Needs review
Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle at Mediacurrent commented
Status: Needs review » Reviewed & tested by the community

I tested #15 and it applies cleanly and works for install and uninstall. Don't know what's up with the testbot but I think this is ready to go so marking as such.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Reviewed & tested by the community » Needs work 
+++ b/xmlsitemap.install
@@ -316,7 +316,7 @@ function xmlsitemap_uninstall() {
+    \Drupal::configFactory()->getEditable('xmlsitemap.settings')->clear($variable);

Why don't we request the config once outside the loop. Or at least the factory once.

Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle at Mediacurrent commented

I don't know... I was just changing the existing code to get past the bug. I don't see any reason why not. I'll roll a new patch with that change later today if no one beats me to it.

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented

Sorry, I wasn't questioning your choice it was more rhetorical. I should have said "lets only request..." Thanks!

Michelle’s picture

Michelle
she/her
Primary language English
Location Wisconsin, USA
CreditAttribution: Michelle at Mediacurrent commented
Status: Needs work » Needs review
FileSize
xmlsitemap-fix-install-uninstall-2427509-21.patch11.8 KB

It installs... It uninstalls... It leaps tall buildings in a single bound...

Ok, maybe not that last part. But it does the first 2 with no errors. :)

And it addresses the rhetorical question. ;)

Michelle

neclimdul’s picture

neclimdul
He/Him
CreditAttribution: neclimdul commented
Status: Needs review » Reviewed & tested by the community

I was waiting for testbot to come back but realized that isn't going to happen. Looks good to me.

paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented

The patch at #21 works for me as well. Thanks @Michelle for picking this up. And thanks @neclimdul for your review as well.

andrei.dincu’s picture

andrei.dincu CreditAttribution: andrei.dincu commented
Status: Reviewed & tested by the community » Fixed
DamienMcKenna’s picture

DamienMcKenna
Location NH, USA
CreditAttribution: DamienMcKenna commented
Status: Fixed » Reviewed & tested by the community

@andrei.dincu: Please don't mark issues "fixed" until the patch is actually committed.

  • paulmckibben committed 3bf9694 on 8.x-1.x authored by Michelle 
    Issue #2427509 by paulmckibben, Michelle, andrei.dincu: Installation...
paulmckibben’s picture

paulmckibben
he/him
Primary language English
Location Atlanta, GA
CreditAttribution: paulmckibben commented
Status: Reviewed & tested by the community » Fixed

Thanks everybody! This patch has been committed to the 8.x-1.x branch.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.