The default base URL being required seems to me to break one of the nice things about Drupal -- that you can move it to a different URL and nothing breaks.

Consider:

1. I go to sitemap settings and change some other settings and save the form
2. Because the form element is required and prepopulated with the default from the site's current location, the site's current location is now saved into the database.
3. I move the site to a new location, say a subfolder, a subdomain, a new domain entirely. Or I just move it from dev to live...
4. The sitemap is now broken, by a setting I didn't even know existed.

Suggestion: leave it blank to automatically use the value from $GLOBALS['base_url'].

CommentFileSizeAuthor
#12 xmlsitemap-base_url_unrequired-1295742-12-D7.patch3.04 KBLiam Morland
#8 1295742.xmlsitemap.xmlsitemap_base_url-unrequired.patch3.1 KBleewillis77
#1 1295742.xmlsitemap.xmlsitemap_base_url-unrequired.patch3.08 KBjoachim
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

joachim’s picture

joachim CreditAttribution: joachim commented
Status: Active » Needs review
FileSize
1295742.xmlsitemap.xmlsitemap_base_url-unrequired.patch3.08 KB

Here's a patch on beta 3.

Note this also catches a bug with xmlsitemap_settings_form_submit missing a & on the $form_state parameter.

Status: Needs review » Needs work

The last submitted patch, 1295742.xmlsitemap.xmlsitemap_base_url-unrequired.patch, failed testing.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Version: 6.x-2.0-beta3 » 7.x-2.x-dev
Category: bug » feature
Priority: Major » Normal

Sorry, you're asking for a new feature otherwise this is "works as designed". You'll need to base your patch on D7 and then we can look at incorporating it into D6 if the feature is approved.

Earnie

joachim’s picture

joachim CreditAttribution: joachim commented
Category: feature » bug

I disagree -- if this is 'works as designed', then the design is wrong, because as I explained in the report, it breaks an important feature of Drupal.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

There are *waaaaaay* too many problems that are easily prevented by making this field required, but this might be something to look at.

leewillis77’s picture

leewillis77 CreditAttribution: leewillis77 commented

We've just been bitten by this again, and Joachim's patch makes sense conceptually to us. If we can get a working patch, would it be considered?

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Category: bug » feature

Yes, all patches are considered. Acceptance would be based on what else breaks because of it.

leewillis77’s picture

leewillis77 CreditAttribution: leewillis77 commented
Status: Needs work » Needs review
FileSize
1295742.xmlsitemap.xmlsitemap_base_url-unrequired.patch3.1 KB

This is a re-roll of Joachim's patch to cleanly apply to 7.x-2.x, and works for us.

mgifford’s picture

mgifford
he/him
Primary language English
CreditAttribution: mgifford at OpenConcept Consulting Inc. commented
Issue summary: View changes

The patch still applies. @leewillis77 are you still using it in production? Your patch from 3 years ago still applies.

leewillis77’s picture

leewillis77 CreditAttribution: leewillis77 at Upbeat Productions commented

Hi;

We don't use it by default on new installs I don't believe - we adjusted our go-live process to avoid the issue (Setting the sitemap up explicitly on production).

Which isn't to say that it's wrong - I don't think it is. It's just that I don't have any hard evidence to back up that conclusion :)

Chris Matthews’s picture

Chris Matthews CreditAttribution: Chris Matthews commented
Status: Needs review » Needs work
Issue tags: +Needs reroll

The 6 year old patch in #8 to xmlsitemap.admin.inc and xmlsitemap.test does not apply to the latest xmlsitemap 7.x-2.x-dev and if still applicable needs a reroll.

Checking patch xmlsitemap.admin.inc...
error: while searching for:
  $form['advanced']['xmlsitemap_base_url'] = array(
    '#type' => 'textfield',
    '#title' => t('Default base URL'),
    '#default_value' => variable_get('xmlsitemap_base_url', $GLOBALS['base_url']),
    '#size' => 30,
    '#description' => t('This is the default base URL used for sitemaps and sitemap links.'),
    '#required' => TRUE,
  );
  $form['advanced']['xmlsitemap_lastmod_format'] = array(
    '#type' => 'select',

error: patch failed: xmlsitemap.admin.inc:326
error: xmlsitemap.admin.inc: patch does not apply
Checking patch xmlsitemap.test...
error: while searching for:
   * Test base URL functionality.
   */
  function testBaseURL() {
    $edit = array('xmlsitemap_base_url' => '');
    $this->drupalPost('admin/config/search/xmlsitemap/settings', $edit, t('Save configuration'));
    $this->assertText(t('Default base URL field is required.'));

    $edit = array('xmlsitemap_base_url' => 'invalid');
    $this->drupalPost('admin/config/search/xmlsitemap/settings', $edit, t('Save configuration'));
    $this->assertText(t('Invalid base URL.'));

error: patch failed: xmlsitemap.test:711
error: xmlsitemap.test: patch does not apply
Liam Morland’s picture

Liam Morland
Primary language English
Location Ontario, CA 🇨🇦
CreditAttribution: Liam Morland at University of Waterloo commented
Title: do not require default base URL » Do not require default base_url
Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
xmlsitemap-base_url_unrequired-1295742-12-D7.patch3.04 KB