Hey there,

I used to have a look on XML sitemap, and this one can not be understood easily, as there is no stylesheets.
You should provide a XML stylesheet, just as XML Sitemap do.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

OwilliwO created an issue. See original summary.

OwilliwO’s picture

Version: 8.x-2.7 » 8.x-2.x-dev
Status: Active » Needs review
FileSize
49.47 KB

Here is a patch to add XSL.
Please have a look and tell me.

OwilliwO’s picture

Title: Add XSL to improve XML sitemap viewing » Better sitemap display needed

Just update issue title.

Status: Needs review » Needs work

The last submitted patch, 2: add-xsl-to-improve-sitemap-review-2834406-2.patch, failed testing.

OwilliwO’s picture

Can somebody help me with this failed test. I do not understand why it fails.
Plus, on my test site, I can add/update/delete custom links in my sitemap without any errors..

testRemoveCustomLinks
fail: [Other] Line 167 of modules/simple_sitemap/src/Tests/SimplesitemapTest.php:
"http://localhost/checkout" not found
OwilliwO’s picture

Status: Needs work » Needs review
FileSize
49.49 KB

New version for the PATCH.
Still one thing to do, I didn't manage to list all translations per link..

Status: Needs review » Needs work

The last submitted patch, 6: add-xsl-to-improve-sitemap-review-2834406-6.patch, failed testing.

gbyte’s picture

Title: Better sitemap display needed » Add XSL for human readability

Thanks for the patch, see my comments below.

  • I understand this is a nice feature for the rare occasion of a human examining the sitemap with their browsers, but as the sitemap is generally intended for bots, a human-readable output is actually not needed. Why is this feature important to you?
  • I assume this code has been copied from xmlsitemap. This is not a problem in itself, but we should think twice how to adapt the code to suit this module.
  • I've seen at least one reeference to xmlsitemap in simple_sitemap.xsl.
  • Shouldn't that tablesorter library be imported by composer? Feels very old school to distribute a copy of it in the module directory.
  • The table sorting does not work for me.
  • Shouldn't the css/js files be defined in a Drupal library (libraries.yml) and attached to the pages?
  • This module doesn't support the changefreq parameter yet so it should be removed from the XSL markup.

I appreciate you iterating on it!

OwilliwO’s picture

Hello!

Thanks for the answer.
I used to install xmlsitemap when developing with Drupal 7, but I discover some malfunctions when testing it with Drupal 8. So I try this one, and one great thing is the link between translated contents.

I may have a look to the generated sitemap, to verify that it's correctly updated on path update for instance, and in Firefox, without XSL, it's very hard to read.

So I copy this part of XMLsitemap module and try tu adapt it.
I'm still trying to improve it and think about removing JS library (even with xmlsitenap I didn't use it).

About "changefreq", I may remove it, until a support of it.

Also, do you have any idea about failed test ? I don't understand what happening about custom links..

OwilliwO’s picture

About ChangeFreq, I post a second patch in which I want to replace this column with a translation set column, in order to display correponding URL in other languages.

OwilliwO’s picture

Only provide a useless patch to perform tests and try to understand why one of them is broken with my patch.
Real patch file come with next comment.

OwilliwO’s picture

Real patch to add an XSL file for a more readable sitemap.
Cross finger for tests..

Status: Needs review » Needs work

The last submitted patch, 12: add-xsl-for-human-readability-2834406-11.patch, failed testing.

gbyte’s picture

This is beginning to look good. I can see it in the next module version. Don't worry about the test, we'll figure it out. I see you removed the js files, can you also remove references to it (I saw some in the controller and in the xsl file)?
Is there anything else you would like to add to the patch? Thanks!

OwilliwO’s picture

New version for XSL readability :

  • no more call to JS
  • simplify XSL sheet, for easier understading/maintainability

Test will probably break again..

Actually, I do not see what I can add..
Maybe a graphic improvment.. But if it's OK to you, it's OK for me now !

Status: Needs review » Needs work

The last submitted patch, 15: add-xsl-for-human-readability-2834406-15.patch, failed testing.

rickdonohoe’s picture

Not sure if I'm adding anything worthwhile here, but just wanted to reply to gbyte.co on why this is useful.

I work with quite a lot of sites and work with a lot of migrations, upgrades and SEO related tasks. I often look at the XML sitemap as it can tell you a few things at a glance:

1. How many results I should expect in Google.
2. An overview of the domain structure used on a site, especially with different content types. Because things are usually done like /news/TITLE and /event/TITLE I can also get a good understanding of the content as it will group it together alphabetically.
3. I can use this to copy the results into a spreadsheet for URL Redirect population purposes, and I can share the XML sitemap with other people to discuss the magnitude and recommendations when it comes to migration; such as what we can ignore, what needs re-written, and what may need to be automatically migrated.

I 'd find all of the above *possible* but very hard without this. It's a great feature in the older XML Sitemap module which hasn't been ported here.

If you want me to help test or anything let me know!

gbyte’s picture

@rickdonohoe Fair enough, I will be altering some of the code in the patch and will be implementing it soon. You guys can iterate on the patch and test it to help speed up things.

gbyte’s picture

Sorry but the patch #15 does not work with the sitemap index. There is simple_sitemap.xsl and simple_sitemap.xsl.bak inside and one works with the sitemap index, the other one works with the sitemaps.

@OwilliwO I suspect it will be easier for you to fix the xsl logic so it works for both the index and the sitemap files. Please create a patch against the latest dev first, as there has been some code clean up.

As I said, I will do the fine tuning as soon as the main functionality is in place. Thanks a lot for your contribution.

OwilliwO’s picture

Hi @gbyte.co

I've just rebuild a site with simple_sitemap and the patch, and I do not see simple_sitemap.xsl.bak file ?
Are you sure you made your tests with correct patch file ?

Plus, can you tell me more about index and sitemap files ? I do not know about this feature, and thought your module provide only one sitemap containing every links. Maybe I'm wrong, just tell me!

gbyte’s picture

@OwilliwO I suspect I recreated the patch after creating the .bak file myself, silly of me :P

Sure, in admin/config/search/simplesitemap you can set up how many links go into one sitemap and if you've got more links than that, a sitemap index gets generated with links to specific sitemaps. You can set this value to something small to test. Your old patch seemed to work with the sitemap index file, the new one does not anymore.

OwilliwO’s picture

Didn't know about this feature.
I'll have a look as soon as possible !

gbyte’s picture

@OwilliwO Any chance you improved the patch since our last exchange?

gbyte’s picture

Version: 8.x-2.x-dev » 8.x-3.x-dev
gbyte’s picture

Nuuou’s picture

I've ported Patch #15 to apply to the 8.x-2.12 branch.
Working nicely for me now after applying the patch. I realize this does not address the issues presented in #19, but hoping it provides another basis to work on since this hasn't been updated in a while.

EDIT: Bad patch, dangit. Look at #27 for fix.

Nuuou’s picture

Fixing previously attached patch (didn't include new XSL files, whoops).

gbyte’s picture

@Nuuou thanks for uploading the patch, but the 2.x version of the module is not receiving feature updates anymore. If you could contribute a patch to 3.x, that would be great.

dksdev01’s picture

Small update to use custom base url. thanks

dksdev01’s picture

Little more twick as base_url was not going to SitemapGenerator but have a call to settings['base_url'].

gbyte’s picture

Component: User interface » Code
Assigned: OwilliwO » Unassigned
Issue tags: -XSL

Thanks, but all new features go to 3.x. If you could help port this patch to 3.x, that would be great. :)

dksdev01’s picture

gbyte’s picture

@dksdev01 The patch does not apply, I presume it is written against a very old version of 3.x. Could you please write a patch against the current 3.x-dev?

WalkingDexter’s picture

Status: Needs work » Needs review
FileSize
29.35 KB

Rewrote the previous patch for the current 3.x-dev. Also added a few changes and improvements. Some features taken from the XML sitemap module.

gbyte’s picture

Status: Needs review » Needs work

Good, clean patch Andrey! Everything seems to work but I have some comments mainly about the sorting functionality.

  • How does 'Translation set' sort, should it not be disabled?
  • How do you feel about 'Priority' being the default sort?
  • In sitemap index where we have '?page=x' (maybe in sitemaps as well?), the sorting should be natural, e.g 1, 2... 10, 11 instead of 1,10, 11, 2.
  • Should jquery tablesorter not be added as a composer dependency instead of included in the module?
  • Some sort of tests would be nice but we can push this before adding tests.
WalkingDexter’s picture

  • Yes, I think, sorting is not required for 'Translation set' and 'Images' columns.
  • Good idea.
  • I don't know whether it is important or not. But yes, your option seems more correct.
  • We can also get the script by using a direct link to GitHub or CDN. Maybe it will be better?
WalkingDexter’s picture

Changes from the previous patch:

  • Added jQuery tablesorter as a composer dependency.
  • The new version of the jQuery tablesorter is used.
  • Disabled sorting for Translation set and Images columns.
  • Priority is the default sorting.
  • Natural sorting for URLs.
  • CSS changes.

Can we push it?

gbyte’s picture

Status: Needs review » Active

@WalkingDexter sounds really good!

As mentioned in another issue, as a maintainer with push access, feel free to push any code to dev if you feel it is worth it and quite stable.
You don't need to consult with me as long as the changes are not dramatic or API changing or requiring an update hook. Thanks for your effort!

  • WalkingDexter authored 9e50dec on 8.x-3.x
    Issue #2834406 by OwilliwO, dksdev01, WalkingDexter, Nuuou, gbyte.co:...
gbyte’s picture

@WalkingDexter now that tablesorter is a composer dependency, what is the correct way to add the library to the project? I believe due to #2494073: Prevent modules which have unmet Composer dependencies from being installed we have to add additional instructions on how to install the module.

This article says it's enough to perform a composer install from the docroot, but I have a drupal composer project install and it did not add the library.

Does it mean we have to run composer from the inside of the module's directory? If yes, the library is downloaded into the module's vendor directory and the reference in code is wrong.

What are your thoughts? If you think the core composer integration for contrib modules is not mature enough, we can go back to just including the library without composer.

WalkingDexter’s picture

If we use composer to load the library, we need to add the package definition to the docroot composer.json file. Otherwise, the tablesorter library and jQuery library will be loaded into the vendor folder.

We can do the same thing as in the Chosen module - to properly install the library, you need to add a package definition in the docroot composer.json file. Please see how this is described in the Chosen README file.

If the above option is not suitable, then we can use a direct link to the tablesorter library (GitHub or CDN) in XSL file.

gbyte’s picture

If we use composer to load the library, we need to add the package definition to the docroot composer.json file. Otherwise, the tablesorter library and jQuery library will be loaded into the vendor folder.

We can do the same thing as in the Chosen module - to properly install the library, you need to add a package definition in the docroot composer.json file. Please see how this is described in the Chosen README file.

I was expecting core to lack a robust composer logic for contrib module dependencies (and for core updates as well), but a definition to docroot composer.json is a bit much for many users - I know this module is not as simple as its name suggests, but we shouldn't go overboard. ;) (Having said that, I also realize, that not installing this dependency is not the end of the world and should be seen as optional.)

If the above option is not suitable, then we can use a direct link to the tablesorter library (GitHub or CDN) in XSL file.

Valid idea. I believe this is the file we need? It's only 43kb so not a big deal. We could do this and then add that thing as composer dependency back as soon as all relevant core composer related issues are taken care of. I've already subscribed to a bunch of them.

The only thing I'm worried about are the EU GDPR regulations. I know this script does not gather any user data, so it should be save to embed it and there should be no need for websites to warn their users about the third party script... Do you know more about this?

What do you personally find to be a better idea, linking to the script, or us commiting it back to the code base (which was your initial plan)?

gbyte’s picture

Status: Active » Fixed

Github messes with their file headers so it is difficult to include js from their pages. I have commited the library for now and we can discuss if we want to switch to a CDN at a later time. Thanks everyone for helping out with this issue, especially @WalkingDexter!

gbyte’s picture

Status: Fixed » Needs work

@WalkingDexter Lasmod sorting does not behave correctly; any idea why this could be?

WalkingDexter’s picture

The only thing I'm worried about are the EU GDPR regulations. I know this script does not gather any user data, so it should be save to embed it and there should be no need for websites to warn their users about the third party script... Do you know more about this?

I know little about it, so it’s hard for me to advise anything.

What do you personally find to be a better idea, linking to the script, or us commiting it back to the code base (which was your initial plan)?

I didn't have any initial plan :) However, I prefer the idea of linking to the script.
For example, we can use this link.

@WalkingDexter Lasmod sorting does not behave correctly; any idea why this could be?

As you can see, standard sorting is applied to this column (perhaps this is the usual string sorting). Maybe there is an example in the tablesorter documentation how to sort by date. If there is no example, we can implement custom handler for Lasmod sorting.

WalkingDexter’s picture

Status: Needs work » Fixed

Fixed sorting for Lastmod column by adding the iso8601date parser for tablesorter plugin. This parser is part of the tablesorter project, but it is located in a separate file. I pushed all changes to the 8.x-3.x. I think we can mark this issue again as fixed.

gbyte’s picture

Great work.

kmajzlik’s picture

Instead of adding custom github composer repository switch to https://asset-packagist.org/package/npm-asset/tablesorter => currently asset-packagist.org is used by most of big Drupal distributions.

gbyte’s picture

Good idea @kmajzlik, however last I checked, running composer install from the webroot directory would not install the contributed module's composer dependencies. Has this changed yet? I've been using the Drupal composer project structure for all my projects, so I cannot verify this ATM.

kmajzlik’s picture

It is bad idea to run composer in webroot. There is no reason to do that. Use Drupal way. And current Drupal way is Composer and Asset packagist.

gbyte’s picture

It is bad idea to run composer in webroot

I meant web root of the Drupal directory. As I said, last time I checked running composer on Drupal's composer.json did not download the dependencies of the contrib modules. But I checked long time ago and it might have changed.

kmajzlik’s picture

It downloads dependencies from repositories defined in top-level composer.json. For example search_api_solr depends on solarium/solarium and it works.
We should mention adding asset-packagist.org as repository to top-level composer.json and then it will work fine.
See:
https://github.com/acquia/lightning/blob/8.x-3.x/composer.json#L24
https://github.com/BurdaMagazinOrg/thunder-project/blob/2.x/composer.jso...
and more...

gbyte’s picture

See but this way we would require more steps when installing a module on top of the regular composer require.... This would be OK if we were dealing with a Drupal distribution, but this is a module (with the term 'simple' in its name).

Is there a way to take advantage of this repository from within the module's composer.json (instead of modifying the root comoser.json) without deviating from good practice?

kmajzlik’s picture

I don't know. I use Composer on all D8 projects (no distributions directly, just inspiring there).

Drush command for manual download is ok for me, but should not be first choice.

BTW: modules like Colorbox, Views Slideshow, Facets and their issues about composer.json are discussing same thing.

gbyte’s picture

I also use exclusively composer based projects. These are based on the drupal composer project. But what I like or dislike is irrelevant.

Problem is, most people expect a composer require module to be the only thing needed to install a module and its dependencies. Requiring them to add a repository to their project manually will add hassle to the process, as they will have to go through the module's documentation.

I propose we keep the libraries committed for a while and continue watching how Drupal handles contributed composer dependencies. #2494073: Prevent modules which have unmet Composer dependencies from being installed is a good start.

If any of you guys disagree, I welcome you to create a new issue regarding this module's dependencies and keep challenging me on my view over there.

Status: Fixed » Closed (fixed)

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