Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
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.
Comments
Comment #2
OwilliwOHere is a patch to add XSL.
Please have a look and tell me.
Comment #3
OwilliwOJust update issue title.
Comment #5
OwilliwOCan 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..
Comment #6
OwilliwONew version for the PATCH.
Still one thing to do, I didn't manage to list all translations per link..
Comment #8
gbyte CreditAttribution: gbyte as a volunteer and commentedThanks for the patch, see my comments below.
I appreciate you iterating on it!
Comment #9
OwilliwOHello!
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..
Comment #10
OwilliwOAbout 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.
Comment #11
OwilliwOOnly 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.
Comment #12
OwilliwOReal patch to add an XSL file for a more readable sitemap.
Cross finger for tests..
Comment #14
gbyte CreditAttribution: gbyte as a volunteer and commentedThis 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!
Comment #15
OwilliwONew version for XSL readability :
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 !
Comment #17
rickdonohoe CreditAttribution: rickdonohoe at Investis Digital commentedNot 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!
Comment #18
gbyte CreditAttribution: gbyte as a volunteer and commented@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.
Comment #19
gbyte CreditAttribution: gbyte as a volunteer and commentedSorry 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.
Comment #20
OwilliwOHi @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!
Comment #21
gbyte CreditAttribution: gbyte as a volunteer and commented@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.Comment #22
OwilliwODidn't know about this feature.
I'll have a look as soon as possible !
Comment #23
gbyte CreditAttribution: gbyte as a volunteer and commented@OwilliwO Any chance you improved the patch since our last exchange?
Comment #24
gbyte CreditAttribution: gbyte as a volunteer and commentedComment #25
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedComment #26
Nuuou CreditAttribution: Nuuou commentedI'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.
Comment #27
Nuuou CreditAttribution: Nuuou commentedFixing previously attached patch (didn't include new XSL files, whoops).
Comment #28
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented@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.
Comment #29
dksdev01 CreditAttribution: dksdev01 as a volunteer and commentedSmall update to use custom base url. thanks
Comment #30
dksdev01 CreditAttribution: dksdev01 as a volunteer and commentedLittle more twick as base_url was not going to SitemapGenerator but have a call to settings['base_url'].
Comment #31
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedThanks, but all new features go to 3.x. If you could help port this patch to 3.x, that would be great. :)
Comment #32
dksdev01 CreditAttribution: dksdev01 as a volunteer and commentedattached patch for 3.x. thanks
Comment #33
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented@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?
Comment #34
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedRewrote the previous patch for the current 3.x-dev. Also added a few changes and improvements. Some features taken from the XML sitemap module.
Comment #35
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedGood, clean patch Andrey! Everything seems to work but I have some comments mainly about the sorting functionality.
Comment #36
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedComment #37
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedChanges from the previous patch:
Can we push it?
Comment #38
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented@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!
Comment #40
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented@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.
Comment #41
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedIf 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.
Comment #42
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedI 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.)
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)?
Comment #43
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedGithub 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!
Comment #44
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commented@WalkingDexter Lasmod sorting does not behave correctly; any idea why this could be?
Comment #45
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedI know little about it, so it’s hard for me to advise anything.
I didn't have any initial plan :) However, I prefer the idea of linking to the script.
For example, we can use this link.
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.
Comment #46
WalkingDexter CreditAttribution: WalkingDexter at Initlab commentedFixed 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.
Comment #47
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedGreat work.
Comment #48
kmajzlik CreditAttribution: kmajzlik at Ciklum Western Europe for BurdaForward commentedInstead 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.
Comment #49
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedGood 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.
Comment #50
kmajzlik CreditAttribution: kmajzlik at Ciklum Western Europe for BurdaForward commentedIt 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.
Comment #51
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedI 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.
Comment #52
kmajzlik CreditAttribution: kmajzlik at Ciklum Western Europe for BurdaForward commentedIt 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...
Comment #53
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedSee 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?
Comment #54
kmajzlik CreditAttribution: kmajzlik at Ciklum Western Europe for BurdaForward commentedI 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.
Comment #55
gbyte CreditAttribution: gbyte as a volunteer and at gbyte commentedI 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.