Skip to content

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

Merged
craftyshaun merged 9 commits intosamdark:masterfrom
ParitoshBh:master
Sep 28, 2021
Merged

Option to include xml stylesheet tag in generated index, sitemap files#63
craftyshaun merged 9 commits intosamdark:masterfrom
ParitoshBh:master

Conversation

@ParitoshBh
Copy link
Copy Markdown
Contributor

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
Copy link
Copy Markdown
Owner

samdark commented Aug 18, 2018

What's the use case for it?

@ParitoshBh
Copy link
Copy Markdown
Contributor Author

ParitoshBh commented Aug 18, 2018

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
Copy link
Copy Markdown
Contributor Author

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

@samdark
Copy link
Copy Markdown
Owner

samdark commented Sep 5, 2018

Not really. It looks OK.

@craftyshaun
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown
Contributor Author

@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
Copy link
Copy Markdown
Owner

samdark commented Sep 25, 2021

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

@samdark
Copy link
Copy Markdown
Owner

samdark commented Sep 25, 2021

But need namespace change reverted, @ParitoshBh.

@samdark
Copy link
Copy Markdown
Owner

samdark commented Sep 25, 2021

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

@craftyshaun
Copy link
Copy Markdown
Collaborator

@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
Copy link
Copy Markdown
Owner

samdark commented Sep 26, 2021

@craftyshaun sent an invite.

@ParitoshBh
Copy link
Copy Markdown
Contributor Author

But need namespace change reverted, @ParitoshBh.

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

@ParitoshBh ParitoshBh mentioned this pull request Sep 28, 2021
@ParitoshBh
Copy link
Copy Markdown
Contributor Author

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

@craftyshaun
Copy link
Copy Markdown
Collaborator

craftyshaun commented Sep 28, 2021

@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 craftyshaun requested a review from samdark September 28, 2021 17:35
@craftyshaun
Copy link
Copy Markdown
Collaborator

@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
Copy link
Copy Markdown
Contributor Author

ParitoshBh commented Sep 28, 2021

@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
Copy link
Copy Markdown
Collaborator

craftyshaun commented Sep 28, 2021

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
Copy link
Copy Markdown
Contributor Author

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
Copy link
Copy Markdown
Owner

samdark commented Sep 28, 2021

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.

Comment thread Index.php Outdated
Comment thread Index.php Outdated
Comment thread Index.php
Comment thread Sitemap.php Outdated
Comment thread Sitemap.php Outdated
Comment thread Sitemap.php Outdated
Comment thread Sitemap.php
@samdark
Copy link
Copy Markdown
Owner

samdark commented Sep 28, 2021

I've applied some cosmetical fixes.

@samdark
Copy link
Copy Markdown
Owner

samdark commented Sep 28, 2021

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

@craftyshaun
Copy link
Copy Markdown
Collaborator

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
Copy link
Copy Markdown
Owner

samdark commented Sep 29, 2021

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

@craftyshaun
Copy link
Copy Markdown
Collaborator

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants