This column will be used to limit the data selected to update the xmlsitemap table. The column will be set to 1 if it needs to update xmlsitemap table during the changes of the data. It will be reset to 0 after xmlsitemap has been updated in the hook_xmlsitemap_links implementation.

Comments

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

This seems different from the use of {xmlsitemap}.status in the 6.x-2.x-dev branch (a TRUE/FALSE flag if the link should be visible in the sitemap). *NOTE*: I'm not yet sure if that's actually what I want to use, so take it with a grain of salt. :)

It sounds like you want it to be a flag for 'needs updating'. Could you walk me through how/why this column would be used?

Anonymous’s picture

Anonymous (not verified) commented

I want to add it to

function xmlsitemap_node_xmlsitemap_links() {
  $query = "SELECT n.nid, n.vid, n.type, n.language, n.uid, n.promote, xn.changed, xn.previously_changed, xn.priority_override, xn.comment_ratio
    FROM {node} n
    INNER JOIN {xmlsitemap_node} xn ON n.nid = xn.nid
    WHERE n.status > 0";

The above query selects every row for the published nodes and then filters below to know to insert or update it. I would add AND (xn.status = 1 OR xn.status IS NULL) to the WHERE clause to select a fewer set of data. The xn.status column will be NULL for new data. The hook_xmlsitemap_links function updates the xmlsitemap.priority column. If there is no change there is no reason to update it. Knowing when the data is changed is a benefit to reduce the DB selections.

As for a status column in the xmlsitemap table; a xmlsitemap.priority of -1 will not appear on the sitemap report already. I don't think a status column just for that purpose is needed.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

Rather then using (xn.status = 1 OR xn.status IS NULL), it would better to use 1 for the default value of the field.
This would reduce the complexity of the SQL query, and would eliminate the hitch I have when I see a field that is NULL by default (:-)).

Anonymous’s picture

Anonymous (not verified) commented

Kiam that doesn't help. The value is NULL because the data join for a row that exists in {node} and not in {xmlsitemap_node}.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

As the query reported uses an inner join, it gets the rows that are present in both the database tables. If you mean to change also the type of join being executed, then what I said is not valid anymore.

I am anyway contrary of using fields whose default value is NULL; NULL has a particular meaning, and it should be used just in left join queries (IMO).
Taking as example the Drupal core code, there are very few cases where a database field has a default value of NULL; I guess there must be a reason.

Anonymous’s picture

Anonymous (not verified) commented

I've not said that the default attribute of the field would be NULL; the SELECT value will be. That aside the Drupal doesn't like NULL default values because not all fields can be and it is easier to construct the select statements because a db_query with '%s' with a NULL becomes '' and that isn't the same thing.

It is common to use NULL default values to reduce the amount of disk space used by the DB. The smarter engines will not output a NULL when the column allows NULL values thus reducing the DB footprint.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

The xn.status column will be NULL for new data

I misunderstood the sentence, then.

For an example of how not proceed, see the 5.x-2 branch code, which has still an open report about a SQL query with a syntax error.

I am not sure it's a good idea to set a field to NULL, if then I have to constantly add a check to verify is the field returned from the query is NULL, and maybe change its value to the one that should be really the default value.
Would you use NULL for the site map priority, when its default value suggested by http://sitemaps.org is 0.5?

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Postponed

Postponed until after the RC release.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: » avpaderno
Status: Postponed » Active

I am going to add the status field, but to use it to tag the rows in the central database table that are enabled, and usable to populate the sitemap.

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Status: Active » Fixed

The field has been added; the code still needs to be changed to use it.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

One step closer to the 6.x-2.x branch now. Are we sure we're adding things just to fix bugs in the 6.x-1.x branch? :)

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented

We are sure, indeed.

The beta3 version still doesn't populate the sitemap (except for me, it seems). I am now changing the code to use just two database tables for all the modules; this would allow the cron tasks to populate the table used to create the sitemap content, which can now be served after a single cron tasks execution.
If I am using a single table, then I need a table field that contains the status of the link, as hook_xmlsitemap_links() will not used anymore; the only time where the modules will be able to update the status of a link is when they execute the Drupal core hooks.

To notice that in the code I started to develop for the 6.x-2branch (see the revision 1.5.2.38.2.16) there was already a similar field, which I before called enabled; if now I am using status is because that is the name given to similar fields in Drupal core tables (see the system, or the node tables).

Status: Fixed » Closed (fixed)

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

avpaderno’s picture

avpaderno
he/him
Primary language Italian
Location Brescia, 🇮🇹 🇪🇺
commented
Assigned: avpaderno » Unassigned
Status: Closed (fixed) » Active

This task needs to be re-opened, now that the code has been rolled back to the point before this task has been implemented.

Anonymous’s picture

Anonymous (not verified) commented
Status: Active » Postponed
Issue tags: +release 6.x-1.1

As I said in #8.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented
Status: Postponed » Closed (won't fix)

Not worth it anymore.