Refactor denyAccessPretty to clear HTML and return to main

[WikiMANNia]

Fix 403 handling and add login links to prevent nginx 404 fallthrough
Co-authored-by: jeffw16 11380894+jeffw16@users.noreply.github.com
ab69f18

[Copilot]

Pull request overview

Refactors ResponseFactory::denyAccessPretty() to ensure the 403 “pretty” denial page replaces any prior output (to avoid upstream/nginx fallthrough behavior) and adds a standard “return to main page” link.

Changes:

• Clear any existing HTML before rendering the 403 denial page (clearHTML()).
• Move the denial message rendering to the end of the method and add returnToMain().

[Copilot]

OutputPage::returnToMain() is now part of the denyAccessPretty flow, but ResponseFactoryTest currently only asserts setStatusCode/addWikiTextAsInterface; consider extending the test to assert returnToMain() is called so this new 403-page behavior is exercised (and to prevent regressions).

[Copilot]

New calls to OutputPage::clearHTML() (and later returnToMain()) will break the unit-test environment that relies on tests/phpunit/namespaced-stubs.php: its OutputPage stub doesn’t define these methods, so createMock() will fail. Update the stub to include clearHTML() and returnToMain(), and (optionally) add expectations in ResponseFactoryTest so the behavior is covered.

[jeffw16]

This PR is currently missing test coverage; please update the tests first so the pipeline can pass.

[jeffw16]

What is the purpose of adding this at the beginning of the page? It's currently unclear what this does, but if there's a good reason for it, an explanatory comment would be much appreciated.

[jeffw16]

Still missing unit test changes

[jeffw16]

This is logic which directly interacts with MediaWiki, so this should be moved back to denyAccessPretty. But, the question remains: why is this necessary?

[WikiMANNia]

I have not the means to do the so called "unit tests". I never use them.

At least the output of
´wfMessage( 'crawlerprotection-accessdenied-title' )´
should be done BEVORE
´wfMessage( 'crawlerprotection-accessdenied-text' )´

isn't it?!??

[jeffw16]

Sorry, but this codebase has a hard dependency on unit tests. As you can see, the checks won't even succeed if the unit tests are not passing. As of last year, these are required in this repo, because in the age of AI coding, we need to ensure the code works even if someone decides to use AI to write the code, not to mention the original point of unit tests was to ensure the code change works as expected.

[WikiMANNia]

I do not understand, why I tiny little extension like CrawlerProtection needs such thing as "unit tests". I've created significantly larger extensions without any unit tests.

because in the age of AI coding, we need to ensure the code works even if someone decides to use AI to write the code, not to mention the original point of unit tests was to ensure the code change works as expected.

Please explain the benefits of all this. I don’t understand what the point of all this unnecessary complexity is.

You’re demanding “unit tests” and justifying this with AI. AI-generated code needs to be tested, right? I predict that if the application code is generated by AI, the “unit tests” will also be generated by AI. And then who’s going to test the “unit tests”?!??

I don’t see any advantage over the traditional method of writing clear and easily understandable code.

[jeffw16]

I do not understand, why I tiny little extension like CrawlerProtection needs such thing as "unit tests".

Any codebase with business logic should ideally have unit tests, no matter how small the business logic is. At the very least, it serves as a sanity check to catch mistakes that might be made during a coding update.

I've created significantly larger extensions without any unit tests.

You are welcome to write your own codebase however you'd like, but MyWikis is aiming to raise our standards so that our code can be considered the gold standard. CrawlerProtection is being used by many large wikis now, and while we make no warranty that it works, I don't want people to get angry at us for things breaking due to poor verification of the code's functionality. The best way to do that is by writing unit and integration tests.

And then who’s going to test the “unit tests”?!??

Unit tests are "tested" by running the test frameworks, but they are verified by humans. Humans working on the codebase are always responsible for reading the unit tests they write (or which AI helps them write).

I don’t see any advantage over the traditional method of writing clear and easily understandable code.

These are not mutually exclusive; code should be both clearly written and testable/tested. In other words, clear code goes hand-in-hand with testing. Please see Robert C. Martin's Clean Code book; it includes testing as an essential component of clean code.

Thanks for your contribution. As I've mentioned, this code can't be merged until the pipelines pass when the unit tests pass. I'm closing this PR until the underlying issues can be resolved. Someone else is also welcome to make a PR to implement this functionality with unit tests.