Refactor/extract page properties #24
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
RobjS
force-pushed
the
refactor/extract-page-properties
branch
from
6f137ab to
7823132
Compare
Currently, we're populating all the data about the post being viewed in the `SendHeaders::pageProperties` property. This is populated by `SendHeaders::getPageProperties()`. It would be a step towards modularity in the plugin if we extracted that data and put it in its own object, so I've added the `Page` class to do that. This commit does not implement the class, it just makes it available. It's also worth noting that I've exactly replicated the logic as it stands (e.g. for returning the post type), even where that logic appears buggy. Instead, I've made a note in the comments of those cases, and we'll fix them in separate commits.
Before: although `SendHeaders` references the global `$post` object, the unit tests for the class never defined that object. They were still passing because the cases tested relied on the properties of that object being unpopulated. Once we'd added tests for the new `Page` class, which defined different global `$post` objects for different cases, those test `$post` objects were then being re-used in the context of the `SendHeaders` tests, because they weren't defining their own `$post` objects. The tests were then failing. By defining an empty global `$post` for all the `SendHeaders` tests, they are now passing again. However, this is a strong indication that the tests for that class are not providing full coverage of every logic path.
This refactor takes the `Page` object we'd created in previous commits and implements it within the `SendHeaders` class. This is a first step towards making this class more modular. I've deliberately made the minimum possible changes to the tests for `SendHeaders` in this commit, in order to demonstrate that this refactor has not made any breaking changes - i.e. all I've done is pass in a mock of the `Page` class (so the constructor doesn't throw an exception), and change one test that mocks a callto a method in `SendHeaders` that no longer exists. While it would be preferable to mock the calls to the `Page` instance (rather than, as is happening now, mocking the calls that happen within the `Page` instance's methods), these minimal changes confirm that the logic within the new `Page` class precisely mirror the logic that was previously within `SendHeaders`.
Before: although we were passing a double of `Page` into `SendHeaders`' constructor, we weren't mocking `Page`'s methods and returns. Instead, the function calls happening inside those methods were mocked. This was useful for confirming that our refactor wasn't breaking anything, but also meant that our unit test wasn't testing a single unit (`SendHeaders`), so was not a sensible long-term testing strategy. Now: we mock the calls to the `Page` instance in our `SendHeaders` tests, meaning our tests are properly focussed on just the single unit under test.
Before: the code was configured to handle either a string or null return from `get_post_type()`. However, `get_post_type()` will never return null, but either a string or `false` Now: we handle a `false` return from `get_post_type()` See: https://developer.wordpress.org/reference/functions/get_post_type/
Before: the `::taxonomies()` method was configured to allow for a null return from `get_post_taxonomies()`. However, it will always return an array, it's just that the array will be empty if there are no post taxonomies. Now: we handle an empty array return. See: https://developer.wordpress.org/reference/functions/get_post_taxonomies/
RobjS
force-pushed
the
refactor/extract-page-properties
branch
from
4b61e2b to
57545d4
Compare
This method no longer gets the page properties, it just optionally outputs the developer meta, so let's rename it accordingly.
RobjS
force-pushed
the
refactor/extract-page-properties
branch
from
57545d4 to
4ed53b8
Compare
snim2
reviewed
Feb 6, 2025
| @@ -6,6 +6,8 @@ | |||
| beforeEach(function () { | |||
| $this->sendHeaders = new \CacheControl\SendHeaders(); | |||
|
|
|||
| global $post; | |||
| $post = (object) []; | |||
There was a problem hiding this comment.
Should this use StdClass? (I notice sometimes we do, sometimes we don't...)
There was a problem hiding this comment.
As I understand it, (object) [] is the same as new stdClass()
php -a
Interactive shell
php > $foo = (object) [];
php > echo get_class($foo);
stdClass
snim2
reviewed
Feb 6, 2025
| @@ -9,12 +9,17 @@ class SendHeaders implements \Dxw\Iguana\Registerable | |||
| protected bool $developerMode = false; | |||
| protected bool $overriddenByTaxonomy = false; | |||
| protected string $currentConfig = 'default'; | |||
| protected array $pageProperties = []; | |||
| protected object $page; | |||
There was a problem hiding this comment.
Why not protected Page $page? Are we not using the new class yet?
snim2
approved these changes
Feb 6, 2025
There was a problem hiding this comment.
This looks sensible and Kahlan runs green for me. I've done a little manual testing on a local env (but not much) and it seems OK.
Note that to run the tests through phpdbg you now need to manually adjust the memory limit as this branch is much more resource hungry than main.
Without adjusting the max memory:
❯ phpdbg -qrr ./vendor/bin/kahlan --coverage=4 --no-colors
_ _
/\ /\__ _| |__ | | __ _ _ __
/ //_/ _` | '_ \| |/ _` | '_ \
/ __ \ (_| | | | | | (_| | | | |
\/ \/\__,_|_| |_|_|\__,_|_| |_|
The PHP Test Framework for Freedom, Truth and Justice.
src directory : /Users/sarah.mount/code/github.com/dxw/saluki-test-site/wp-content/plugins/dxw-cache-control/src
spec directory : /Users/sarah.mount/code/github.com/dxw/saluki-test-site/wp-content/plugins/dxw-cache-control/spec
................................................................. 65 / 90 ( 72%)
...................
[PHP Fatal error: Allowed memory size of 134217728 bytes exhausted (tried to allocate 65536 bytes) in .../dxw-cache-control/vendor/kahlan/kahlan/src/Jit/TokenStream.php on line 186]and with:
❯ phpdbg -d memory_limit=-1 -qrr ./vendor/bin/kahlan --coverage=4 --no-colors
_ _
/\ /\__ _| |__ | | __ _ _ __
/ //_/ _` | '_ \| |/ _` | '_ \
/ __ \ (_| | | | | | (_| | | | |
\/ \/\__,_|_| |_|_|\__,_|_| |_|
The PHP Test Framework for Freedom, Truth and Justice.
src directory : .../dxw-cache-control/src
spec directory : .../dxw-cache-control/spec
................................................................. 65 / 90 ( 72%)
......................... 90 / 90 (100%)
Expectations : 231 Executed
Specifications : 0 Pending, 0 Excluded, 0 Skipped
Passed 90 of 90 PASS in 0.534 seconds (using 154MB)
Coverage Summary
----------------
Lines %
\ 135 / 149 90.60%
└── CacheControl\ 135 / 144 93.75%
├── Options 44 / 44 100.00%
│ ├── Options::addActionLinks() 3 / 3 100.00%
│ ├── Options::addOptions() 9 / 9 100.00%
│ ├── Options::addOptionsPage() 1 / 1 100.00%
│ ├── Options::getPostTypesConfig() 12 / 12 100.00%
│ ├── Options::getTaxonomiesConfig() 10 / 10 100.00%
│ ├── Options::getTemplatesConfig() 6 / 6 100.00%
│ └── Options::register() 3 / 3 100.00%
├── Page 13 / 14 92.86%
│ ├── Page::isAdmin() 1 / 1 100.00%
│ ├── Page::isArchivePage() 1 / 1 100.00%
│ ├── Page::isFrontPage() 1 / 1 100.00%
│ ├── Page::isHomePage() 1 / 1 100.00%
│ ├── Page::isLoggedInUser() 1 / 1 100.00%
│ ├── Page::isPreviewPage() 1 / 1 100.00%
│ ├── Page::postId() 3 / 3 100.00%
│ ├── Page::postType() 1 / 1 100.00%
│ ├── Page::requiresPassword() 1 / 2 50.00%
│ ├── Page::taxonomies() 1 / 1 100.00%
│ └── Page::templateName() 1 / 1 100.00%
└── SendHeaders 78 / 86 90.70%
├── SendHeaders::__construct() 1 / 1 100.00%
├── SendHeaders::getContext() 2 / 2 100.00%
├── SendHeaders::getPageConfiguration() 46 / 54 85.19%
├── SendHeaders::outputDeveloperMeta() 11 / 11 100.00%
├── SendHeaders::register() 2 / 2 100.00%
└── SendHeaders::setCacheHeader() 16 / 16 100.00%
Total: 90.60% (135/149)
Coverage collected in 0.017 secondsNote that on main without adjusting the memory limit you get very slightly more code coverage:
Passed 73 of 73 PASS in 0.070 seconds (using 25MB)
Coverage Summary
----------------
Lines %
\ 126 / 139 90.65%
└── CacheControl\ 126 / 135 93.33%
├── Options 44 / 44 100.00%
│ ├── Options::addActionLinks() 3 / 3 100.00%
│ ├── Options::addOptions() 9 / 9 100.00%
│ ├── Options::addOptionsPage() 1 / 1 100.00%
│ ├── Options::getPostTypesConfig() 12 / 12 100.00%
│ ├── Options::getTaxonomiesConfig() 10 / 10 100.00%
│ ├── Options::getTemplatesConfig() 6 / 6 100.00%
│ └── Options::register() 3 / 3 100.00%
└── SendHeaders 82 / 91 90.11%
├── SendHeaders::getContext() 2 / 2 100.00%
├── SendHeaders::getPageConfiguration() 46 / 54 85.19%
├── SendHeaders::getPageProperties() 12 / 12 100.00%
├── SendHeaders::getPostId() 3 / 3 100.00%
├── SendHeaders::hasPassword() 1 / 2 50.00%
├── SendHeaders::register() 2 / 2 100.00%
└── SendHeaders::setCacheHeader() 16 / 16 100.00%
Total: 90.65% (126/139)
Coverage collected in 0.015 seconds
I think this is just a result of the increase in the number of lines of code, rather than an actual reduction in test coverage? Though I can't work out why it thinks |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR refactors the
SendHeadersclass to extract the page data out to a separatePagemodel that holds all the information about what kind of page is currently being viewed. This is hopefully the first step in a large refactor to make the plugin's code more modular and developer-friendly.Essentially, the refactor takes all the logic that was previously put into the
SendHeaders::pagePropertiesproperty and moves it to thePageclass. This is demonstrated in commitf42b7b9, which implements the new class, but does not amend the tests to mock the calls to the new class's methods. Instead, the existing test mocks are retained (which mock the logic that now happens withinPage's methods). The fact that the tests all continued to pass demonstrates that no changes to the overall behaviour have occurred. The tests were then refactored fully in the following commit, so that the tests are only examining the singleSendHeadersclass under test.I have fixed a couple of minor bugs that emerged during the refactor, where we were expecting the wrong kind of return type if
get_post_type()orget_post_taxonomies()fail, but these are edge cases that should not affect standard behaviour of the plugin.How to test
composer install && vendor/bin/php-cs-fixer fix --dry-run & vendor/bin/psalm && vendor/bin/kahlan specto run the testsno-cacheheaders are always served if you're e.g. logged in, or viewing a preview