Support from Acquia helps fund testing for Drupal Acquia logo

Comments

vytch created an issue. See original summary.

vytch’s picture

Title: Changereq implementation missing » changefreq implementation missing
vytch’s picture

vytch’s picture

Fixing the notice message when the changefreq is not yet defined

nikunjkotecha’s picture

Status: Active » Needs review
FileSize
19.94 KB

Adding patch with all required changes to add changefreq element.

vytch’s picture

If a node has been already treated as a menu_link_content, then it does not get the data set from the node edit page.

vytch’s picture

Fixing issue with notice error.

gbyte’s picture

Version: 8.x-2.9 » 8.x-2.x-dev
Status: Needs review » Needs work

Thanks for your input guys. @vytch Can you incorporate your change into #5? It seems that one is more complete. I am going to review it in a few moments.

gbyte’s picture

@nikunjkotecha thanks for yout patch. I have found a couple of issues that need fixing before we commit.

  1. We need an update hook to alter existing data, as the sitemap threw an error when I ran indexation after applying the patch. I believe we need to alter the db schema of simple_sitemap_entity_overrides and configuration storage of simple_sitemap.bundle_settings.* and simple_sitemap.custom.
  2. This is probably related, I'm getting Notice: Undefined index: changefreq in Drupal\simple_sitemap\Simplesitemap->setEntityInstanceSettings() (line 418 of Simplesitemap.php). when trying to override entity instance.
  3. This is probably also related, when visiting Custom links after applying the patch, the paths are formatted incorrectly in the textbox. There must be an (optional) way of adding changefreq for custom links and the value added needs to be validated.
  4. Line 314 of the patch doesn't seem to make sense: + ? $custom_link['path'] . ' ' . $this->formHelper->formatPriority($custom_link['changefreq']). There should be a formatChangefreq function I believe.

Another question is, what do you think about implementing automatic changefreq values for entities (as we have last viewed and created data) as xmlsitemap does?

Thanks!

rli’s picture

According to the comment in #9, re-rolled the patch in #5 to fix following:

  1. Added update hook to add a column simple_sitemap_entity_overrides to fix the PHP warnings. The config storage is already changed in the patch in #5.
  2. Notice: Undefined index: changefreq in Drupal\simple_sitemap\Simplesitemap->setEntityInstanceSettings() (line 418 of Simplesitemap.php). is gone after altered the db.
  3. Made changes in helper function to store changefreq for custom links and display changefreq properly in form.
  4. No need to use formatChangeFreq formatter as it is just string selected from dropdown list.

Have not got time to think about the automatic changefreq generation yet. The options allows user to control the value per content type is good enough for our project at the moment.

rli’s picture

Status: Needs work » Needs review

The last submitted patch, 4: adding-changefreq-2875538-comment-12069822.patch, failed testing. View results

The last submitted patch, 3: adding-changefreq-2875538-comment-12069821.patch, failed testing. View results

rli’s picture

Status: Needs review » Needs work

Seems no need to add column in db as the settings are stored as serialised array.

rli’s picture

Added update hook to alter all existing data.

rli’s picture

Status: Needs work » Needs review
FileSize
23.22 KB

Rerolled patch to alter all existing settings.

  • gbyte.co committed 30805e7 on 8.x-2.x
    git commit -m 'Issue #2875538 by vytch, rli, nikunjkotecha, gbyte.co:...
gbyte’s picture

Status: Needs review » Fixed

Thanks for your patches guys, couldn't use them much however. I've committed all changes to dev, please take a look for me.

Status: Fixed » Closed (fixed)

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