samdark / sitemap

Sitemap and sitemap index builder
BSD 3-Clause "New" or "Revised" License
534 stars 86 forks source link

Option to include xml stylesheet tag in generated index, sitemap files #63

Closed ParitoshBh closed 3 years ago

ParitoshBh commented 6 years ago

Added a function for appending xml stylesheet to generated files (index and child). Example,

$sitemap->setStylesheet($stylesheetUrl)

Output,

<?xml version="1.0" encoding="UTF-8"?>
<?xml-stylesheet type="text/xsl" href="<STYLESHEET_URL>"?>
<sitemapindex xmlns="http://www.sitemaps.org/schemas/sitemap/0.9">

There's some code duplication but I noticed work is going for v 3.0.0 release and this can be modified/fixed as per new code standards!

samdark commented 6 years ago

What's the use case for it?

ParitoshBh commented 6 years ago

I am using your library for generating sitemap with over 100,000 url's in it (multi-lingual) and at times it gets pretty hard to debug to ensure generated output is proper XML (except for the obvious exceptions raised by the library itself). This basically gives the ability to quickly glance over to see if the sitemap has all the expected url's.

Needless to say, this might not really be useful for the smaller sites but having an option doesn't hurt!

Edit: There's the presentation part too. Showing it a person, not really technical (read upper management, etc).

ParitoshBh commented 6 years ago

Just following up, is there something amiss from this PR? If there is please do share!

samdark commented 6 years ago

Not really. It looks OK.

craftyshaun commented 3 years ago

Hi @samdark is there any plan to merge this? I'd love to add the stylesheet to the URL to get some formatting. It seems dev has kinda stalled. Are you looking for contirbuters?

ParitoshBh commented 3 years ago

@craftyshaun If it is of any help, I forked the repo and merged the changes. Here's the release https://github.com/ParitoshBh/sitemap/releases/tag/2.2.2 and installation step is here https://github.com/ParitoshBh/sitemap#installation

samdark commented 3 years ago

@craftyshaun sure, can merge it. Somehow lost it in the notifications back then :)

samdark commented 3 years ago

But need namespace change reverted, @ParitoshBh.

samdark commented 3 years ago

@craftyshaun if you'd like to take care of the library and finish 3.0.0, I can add you to maintainers.

craftyshaun commented 3 years ago

@samdark I'd be happy to have a go at finishing v3

I want to do more open source and this lib looks like the best sitemap tool for PHP so it's a good place to put some effort.

samdark commented 3 years ago

@craftyshaun sent an invite.

ParitoshBh commented 3 years ago

But need namespace change reverted, @ParitoshBh.

I'll revert the changes and update this PR accordingly.

ParitoshBh commented 3 years ago

@samdark Reverted changes for namespace. Good for re-review :)

craftyshaun commented 3 years ago

@ParitoshBh I'd also like your thoughts to maybe include an example XSL in the project. This could then be linked in the README.md

We could use this as a starting point .

I'd suggest we add some form of attribution back to the original author in the footer but add up the top that the style sheet has been generated by this plugin with a link back to the repo.

On the note of attribution adding generator tags as comments to the generated XML is possible as well. I think anything we can do to get the word out about the library the better.

I'll open another issue for this any maybe look at it for v3

craftyshaun commented 3 years ago

@samdark Reverted changes for namespace. Good for re-review :)

@ParitoshBh looks good to me!

@samdark added me so I can merge. I've requested a review as it's the first change.

If he has no issues or doesn't respond in 24 hours I'll merge and create a new minor point release which will flow to composer etc.

Thanks for completing this PR after it was left for so long.

ParitoshBh commented 3 years ago

@craftyshaun I don't think there's a need to rely on another project for sample XSL. It isn't hard to generate, here's a sample file that I think should be good to include in this repo,

sitemap.txt

Please change extension from .txt to .xsl (Github doesn't allow uploading .xsl)

As for the attribution, personally I am not a fan. There's already too many attributions floating in FOSS around but if there's consensus to add one, the first <p class="expl"></p> will be a good location.

craftyshaun commented 3 years ago

Thanks for the example. My main reason for the other one is it might have been 'nicer looking' but simple does the job considering it's mostly for bots

The whole attribution thing is hard and I agree it's a slippery slope. FOSS takes time to maintain and also requires a community and attribution helps grow that community and show the time (via GitHub logs).

As it's @samdark who created the project I'll let him break the tie on of we should inclide a back link to the repo in the example XSL.

Once again thanks for your effort, I can't wait to merge this so I can use it in my new project.

ParitoshBh commented 3 years ago

Thanks for the example. My main reason for the other one is it might have been 'nicer looking' but simple does the job considering it's mostly for bots

Agreed on the nicer looking part. This is the reason for creating this PR 😉 I didn't want a boring sitemap!

The whole attribution thing is hard and I agree it's a slippery slope. FOSS takes time to maintain and also requires a community and attribution helps grow that community and show the time (via GitHub logs).

As it's @samdark who created the project I'll let him break the tie on of we should inclide a back link to the repo in the example XSL.

Fair enough 👍

samdark commented 3 years ago

As it's @samdark who created the project I'll let him break the tie on of we should inclide a back link to the repo in the example XSL.

Won't hurt. I'm all for it.

samdark commented 3 years ago

I've applied some cosmetical fixes.

samdark commented 3 years ago

I'd love to see some unit tests validating behavior but that's up to you. Could be merged w/o it.

craftyshaun commented 3 years ago

I'd love to see some unit tests validating behaviour but that's up to you. Could be merged w/o it.

I was planning to add these covering tests as part of v3 or as an additional issue post-merge. I'll create an issue for the example sitemap and further tests (assuming it is not going to be re-written in 3.x)

samdark commented 3 years ago

I've tagged a new release: https://github.com/samdark/sitemap/releases/tag/2.3.0

craftyshaun commented 3 years ago

I've tagged a new release: https://github.com/samdark/sitemap/releases/tag/2.3.0

Thanks I was going to wait until I had tests in a few days.