Skip to content

Make the PHAR "website" a little more fancy #1156

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 11 commits into
base: gh-pages
Choose a base branch
from

Conversation

benno5020
Copy link
Contributor

This PR updates the gh-pages branch by styling the pages and adding a script to generate a listing of all downloadable phars.

Description

See #107

  • The pages are responsive and provide a light and dark mode.
  • I did not want to add any bloat so there is no build tooling at all.
  • I have prefixed the template and the php script with an underscore because I believe that will keep them from being served by GitHub Pages. Please verify that this behaves as expected.
  • I have linked the sources for the icons in my commit messages.
  • Please note that phars.html displays the git committer date of each file (https://git-scm.com/docs/pretty-formats). I wasn't sure what else to show as filemtime would not be right.
  • I did not look into generating phive.xml (as mentioned here: Make the PHAR "website" a little more fancy #107 (comment)). The filenames do not always match the respective version (note the dash in e.g. <release version="4.0.0-RC1" url="https://phars.phpcodesniffer.com/phars/phpcs-4.0.0RC1.phar">) and I didn't want to make any assumptions.
  • The php script is just something I quickly whipped up. I intentionally did not make it pass PHPStan's analysis because I believe that would make the code less readable.

Personally, I like the look of phars.html but index.html could still use some work. But in the interest of keeping things moving, I thought I should send this PR sooner rather than later.

Suggested changelog entry

n/a

Related issues/external references

Fixes #107

Types of changes

(I'm not sure if this applies for the gh-pages branch.)

  • Bug fix (non-breaking change which fixes an issue)
  • [] New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
    • This change is only breaking for integrators, not for external standards or end-users.
  • Documentation improvement

PR checklist

  • I have checked there is no other PR open for the same change.
  • I have read the Contribution Guidelines.
  • I grant the project the right to include and distribute the code under the BSD-3-Clause license (and I have the right to grant these rights).
  • I have added tests to cover my changes.
  • I have verified that the code complies with the projects coding standards.
  • [Required for new sniffs] I have added XML documentation for the sniff.

@jrfnl
Copy link
Member

jrfnl commented Jul 4, 2025

@benno5020 Thank you so much for doing this!

I haven't reviewed the code in depth (yet), but looking at the principle, there are a couple of things I'd like to get your opinion on.

  • The pages are responsive and provide a light and dark mode.

💞

  • I have linked the sources for the icons in my commit messages.

Thanks for doing that 👍🏻

  • I have prefixed the template and the php script with an underscore because I believe that will keep them from being served by GitHub Pages. Please verify that this behaves as expected.

I don't believe this assumption is correct. Underscore prefixed pages do not get published when the site is build via the standard Jekyll publishing mechanism, except that is explicitly disabled for this GH Pages site via the .nojekyll file.

  • I did not want to add any bloat so there is no build tooling at all.
  • Please note that phars.html displays the git committer date of each file (https://git-scm.com/docs/pretty-formats). I wasn't sure what else to show as filemtime would not be right.
  • The php script is just something I quickly whipped up. I intentionally did not make it pass PHPStan's analysis because I believe that would make the code less readable.

Well, the PHP script is kind of a build tool 😉

To me, it feels counter-intuitive to have an auto-generated phars.html file committed to the repo, when there is a script available to auto-generate the file.
The risk of the committed file going out of date, because manually running the update is forgotten, feels unnecessary.

Side-note: I wonder whether the phars.html page should in actual fact be the phars/index.html page ?

So... what I'm currently thinking is this:

  • Re-organize the files in the gh-pages branch like so:
    • root
      • CNAME
    • .github/workflows
      • publish-website.yml
      • the generate_phars_list.php script and the phars.html.template file (or possibly have the script in a bin directory or both files in a build directory ?)
    • src
      • the other pre-existing files, including your updates + the new styles.css file (not the phars.html file!)
  • Create a publish-website.yml workflow to run the generate_phars_list.php script and create the phars.html (or phars/index.html) file and deploy the website from the src directory on pushes to the branch.
  • Possibly use the GH API to retrieve the publish date of the relevant release for each PHAR file ? (to get round the modified/committer date issue).
  • The .nojekyll file would be removed as it would (should) no longer be necessary once the site is switched to a custom deploy script.

How does that sound to you ? Do you see any problems which I'm overlooking ?

Now, I don't know whether you have any experience with creating a custom GH Pages deploy script ?
If not, I'd be happy to whip something up myself. Or, if you prefer, coach you through creating the deploy script.

If we would decide to go down this road, I think the file moves need to be done in a separate commit, probably as the first commit for this PR, to allow for the best chance of git keeping track of the file history correctly.

  • I did not look into generating phive.xml (as mentioned here: Make the PHAR "website" a little more fancy #107 (comment)). The filenames do not always match the respective version (note the dash in e.g. <release version="4.0.0-RC1" url="https://phars.phpcodesniffer.com/phars/phpcs-4.0.0RC1.phar">) and I didn't want to make any assumptions.

That's totally fair and something which can be left as "future scope" to be added to the publish workflow at a later date.

Personally, I like the look of phars.html but index.html could still use some work. But in the interest of keeping things moving, I thought I should send this PR sooner rather than later.

Iteration is good ;-)

@benno5020
Copy link
Contributor Author

benno5020 commented Jul 6, 2025

Hi @jrfnl,
thank you for your feedback!

  • I just tried it out and you are correct, files prefixed with an underscore are still served.
  • I agree that generated files should (generally) not be committed, so I'm very much in favour of a workflow.
  • We could definitely query the GitHub API, but then we'd have to think about the (unlikely) scenario that the API is down during deployment.

I have restructured the files and created a workflow.
You can see it in action here: https://github.com/benno5020/PHP_CodeSniffer/actions/runs/16099172225/job/45426029117 (Note that the linked workflow is not 100% identical to the one in this branch. I have since split up the PHP step.)
The resulting pages can be viewed here: https://benno5020.github.io/PHP_CodeSniffer/

Copy link
Member

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @benno5020 Thank you for your continued work on this!

  • We could definitely query the GitHub API, but then we'd have to think about the (unlikely) scenario that the API is down during deployment.

True - in that case, maybe we should leave the "date" bit for the next iteration and discuss that separately ?

My thoughts on this at this moment:

  1. We could check the GH API, and if there is no usable response fall back to the original logic ?
  2. Alternatively, we could check the status of the GH API from within the workflow using the https://github.com/crazy-max/ghaction-github-status action runner and fail the workflow run if the GH API has a partial_outage (or higher).
    That would prevent deploying the phar site in a broken state and would allow for "Re-running failed build(s)" with just a few clicks once the API outage has resolved.

Side-note: we may want to use that action runner in the workflow either way to check for an outage of GH Pages before deploying...

I have restructured the files and created a workflow. You can see it in action here: https://github.com/benno5020/PHP_CodeSniffer/actions/runs/16099172225/job/45426029117 (Note that the linked workflow is not 100% identical to the one in this branch. I have since split up the PHP step.) The resulting pages can be viewed here: https://benno5020.github.io/PHP_CodeSniffer/

Great work!

I've left some questions and remarks inline.

If you'd like to see an example of a workflow which is using most of what's being discussed, you could have a look at the workflow in use for the schema.phpcodesniffer.com website: https://github.com/PHPCSStandards/schema.phpcodesniffer.com/blob/stable/.github/workflows/update-website.yml

There is one important difference between that website and this one - the schema website does not use the gh-pages branch, but is a repo specifically for that website. Other than that, the principles are very similar.

Feels like we're getting very close now!


on:
push:
branches: ["gh-pages"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nitpick for readability:

Suggested change
branches: ["gh-pages"]
branches:
- "gh-pages"

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to also suggest to do a "dry-run" on pull requests to the gh-pages branch, i.e. run the script, but don't deploy.

To enable this, the script does need some tweaks. The example script I mention above for the "schema" website has these tweaks already in place.

Alternatively, we could leave this for a future iteration.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a separate build job for pull requests.

Comment on lines 32 to 35
- name: Install PHP
run: |
sudo apt-get update
sudo apt-get install -y php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just curious: why are you installing PHP like this ?

IIRC, all actions images already come with a semi-recent PHP version pre-loaded, so if we don't want any control over the PHP version, we could just use that.
Ref: https://github.com/actions/runner-images/blob/main/images/ubuntu/Ubuntu2404-Readme.md#php-tools

Alternatively, if we'd want more control over the PHP version and PHP ini settings and such, I'd recommend installing PHP using the shivammathur/setup-php action runner.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was no particular reason. I assumed the image did not come with PHP and did not see a reason not to trust the apt repository or care about the exact php version or settings.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the code to use shivammathur/setup-php so it's consistent with schema.phpcodesniffer.com's workflow.

cancel-in-progress: false

jobs:
deploy:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I can imagine this would inhibit testing at this time, for GH Pages deployments, I generally recommend having a job-level condition to only run/deploy when the workflow is triggered in the original repo.

This prevents lots of semi-duplicate GH Pages websites sprouting up when people contribute and reduces the risk of the CNAME being hijacked.

    # Don't run on forks.
    if: github.repository == 'PHPCSStandards/PHP_CodeSniffer'

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

.gitignore Outdated
Comment on lines 1 to 3
vendor
.phpunit.result.cache
.idea
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I'm missing something, none of these have a risk of existing with the repo as it is at this time, so there is no reason for these to be .gitignored.

Side-note: .idea never belongs in .gitignore, that should always be a user specific global ignored directory.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They existed in my folder when I git switched from the master branch so I thought it wouldn't hurt to ignore them.

Regarding the side-note: Interesting, I hadn't heard of this. There seem to be different schools of thought:
Does not ignore .idea:

Ignores .idea in some form:

Sorry, this list got out of hand. I tried to find more projects in support of your argument but kept running into projects that do ignore .idea. I don't care as much as this list would indicate ;)

Copy link
Member

@jrfnl jrfnl Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They existed in my folder when I git switched from the master branch so I thought it wouldn't hurt to ignore them.

Okay, for vendor and the PHPUnit cache file I can imagine this, if the branch is checked out in a local fork also used for PHPCS itself, so fair. (I personally always created a separate local fork for gh-pages as switching between orphan branches can really mess things up)

As for the .idea folder: the fact that lots of projects ignore it, doesn't mean that's correct.
Different people use different editors/IDEs and that's a user-preference and unrelated to any specific project. That's why this should be ignored via the user-specific .gitignore_global file, not via a project .gitignore (unless a project forces you to use a specific IDE, but I can hardly imagine any project would do so).

Copy link
Contributor Author

@benno5020 benno5020 Jul 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the fact that lots of projects ignore it, doesn't mean that's correct.

Agreed, and I do see your point.

.gitignore Outdated
vendor
.phpunit.result.cache
.idea
src/phars/index.html
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As a rule of thumb, I'd ask you to end every file on a new line character (here and elsewhere).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you think it would be a good idea to add this instruction to the codebase somehow? I'm not sure how much tooling you see fit. Personally, I'd add an

# .editorconfig

root = true

[*]
insert_final_newline = true
end_of_line = lf

(I do, of course, see that this would not technically enforce the rule.)

And come to think of it, wouldn't a .gitattributes file containing * text=auto eol=lf make sense as well?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.gitattributes 👍🏻

As for an .editorconfig, I have no objections against it, though I also don't see much use for it. Fine if others find it useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added .gitattributes but did not add an .editorconfig. I agree there's not much use for it if it's not strictly enforced.

Comment on lines 1 to 2

<?php
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<?php
#!/usr/bin/env php
<?php

Might also be a good idea to have a minimal file docblock, which at the very least identifies the file as part of PHP_CodeSniffer + annotates the minimum PHP version for this script (as it doesn't have to comply with the same PHP requirements the main package has).

Maybe something along the lines of:

/**
 * Script to auto-generate the `phar/index.html` page for the GH Pages website.
 *
 * {@internal This script has a minimum PHP requirement of PHP ...}
 *
 * @copyright 2025 PHPCSStandards and contributors
 * @license   https://github.com/PHPCSStandards/PHP_CodeSniffer/blob/master/licence.txt BSD Licence
 */

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I think the minimum PHP version should be PHP 7 because I used some type hints, but those could be removed if need be.

src/index.html Outdated
</div>
<aside class="additional-info">
<a href="./phars/">
See all PHAR files
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We may need to think about this phrase a little.

The PHAR website doesn't contain all previously released PHAR files. Only the ones released since PHIVE support was added (and the PHAR files started to be signed).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've renamed the page "PHAR Archive" and added a footnote.

@@ -0,0 +1,200 @@
@import url(https://fonts.bunny.net/css?family=open-sans:400,800);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a strong reason to use an external font file ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, just convenience. I did pick bunny over Google fonts for GDPR reasons but did not want to look into font licenses when I wrote that bit of code.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Appreciate the GDPR consideration. I was more wondering why an external font is used at all, I mean, why not use the OS system fonts ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fanciness! Seriously though, it would have felt strange to not set a font when the goal is to achieve a more professional look. But I'm open to your suggestions :)

If you'd like to stick with Open Sans, I believe we could self-host it after downloading WOFF2 files from here: https://gwfh.mranftl.com/fonts/open-sans?subsets=latin
Here's the license: https://github.com/googlefonts/opensans/blob/main/OFL.txt

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

Successfully merging this pull request may close these issues.

2 participants