Skip to content

Revert Rector integration and loosen up some of the typings#160

Open
rhendrickson-sc wants to merge 18 commits intomainfrom
fix/revert-rector
Open

Revert Rector integration and loosen up some of the typings#160
rhendrickson-sc wants to merge 18 commits intomainfrom
fix/revert-rector

Conversation

@rhendrickson-sc
Copy link
Copy Markdown
Contributor

Reviewing this

Unfortunately this PR looks like it's undoing everything that Rector did. While that's true, it might also be worthwhile to view the individual commits to see what changed on a more specific level besides "Rector was removed".

If you have any questions or hesitations about this PR, please don't hesitate to reach out.

Issue

The project had quite a few Dependabot warnings about gitbook-cli. We also wanted to update the site overall.

Solution

Updated and removed the packages in composer.json. Most of the dependencies were updated, however I removed some that weren't being used and/or were causing conflicts/weren't necessary anymore after updating another dependency.

Replaced gitbook-cli entirely with VitePress. Note: this does have the "negative" that the documentation site will not longer have the "Api References" section because the libraries I could find that would do something like that were either incompatible with VitePress, or were last updated five+ years ago.

Impact

Should not have any impact on anyone. Types weren't tightened and Rector wasn't installed, so should run a risk of introducing breaking changes without a more planned breaking change.

Usage Changes

There are no usage changes besides the documentation now being handled via VitePress instead of GitBook.

Testing

Running lando unit and lando sniff both have passing results.

Package is no longer maintained and should be replaced with
php-stubs/acf-pro-stubs
This package is horrendously out of date since Coby left SiteCrafting,
and it isn't being maintained anymore.

Felt like removing the package was the best path forward instead of
forking it and having to update it.

It has a conflict with behat/behat, as they both require `symfony/yaml`,
 but this acobster/wp-cli-yaml-fixtures wants `symfony/yaml:3.4` and the
  newer versions of `behat/behat` want `symfony/yaml:7.x`.
Fix an "unmaintained packaged" warning when running `lando composer
audit`.
Set it to be ^6.5.5 so it can pull the latest minor version on composer
updates.
Remove squizlabs/php_codesniffer because wp-coding-standards/wpcs
requires it, so it makes little sense for us to also require it
when we're not using it directly.
This package is no longer maintained and no longer works on Node
versions 10 through 22.

After talking to Scott about it, we decided it was better to just strip
it out for now, as it's probably seldom being used.
We were only using this to dynamically generate the API Reference
section of the documentation website.
However, this package, and the others like it, are seldom updated, so it
 was more hassle than it was worth to try and keep it.
Now the documentation site is blazingly fast 🦀
### Benevolent Dictator

- [Coby Tamayo](https://github.com/acobster/)
- [Scott Dunham](https://github.com/sdunham)
Copy link
Copy Markdown
Contributor Author

@rhendrickson-sc rhendrickson-sc Mar 2, 2026

Choose a reason for hiding this comment

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

@sdunham Also just a heads up that I removed Coby from the "Benevolent Dictator", being he hasn't been an employee at SiteCrafting for a couple years now. I put you there because it made the most sense to me, but I'm open to modifying this file however you want.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR reverts Rector integration and "loosens" PHP typings back to a pre-Rector state, while also updating several dependencies and migrating the documentation site from GitBook to VitePress.

Changes:

  • Removed Rector configuration, declare(strict_types=1) declarations, and constructor property promotion across all PHP source files and tests; replaced arrow functions and first-class callables with regular closures.
  • Updated composer.json and package.json dependencies; removed rector/rector and bumped VitePress/Vue/Behat/WPCS versions.
  • Migrated docs to VitePress format, updated doc content, team roster, and changelog.

Reviewed changes

Copilot reviewed 80 out of 84 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
lib/Conifer/Admin/Page.php Loosened types; do_add() return type changed from Page to void; render call changed to pass string instead of array (bug)
lib/Conifer/Admin/SubPage.php Same loosening as Page.php; same render call bug
lib/Conifer/Alert/DismissableAlert.php Converted to explicit property declarations; docblock @param type error for $options
lib/Conifer/Notifier/SimpleNotifier.php Removed constructor promotion; docblock @param has wrong parameter name ($o vs $to)
lib/Conifer/Post/BlogPost.php Loosened types, closures replace arrow functions; removed .to_array() on get_related() result
lib/Conifer/Post/FrontPage.php Changed Timber::get_post() to new static()
lib/Conifer/Post/Page.php Loosened types; get_title_from_nav_or_post() now passes $this to get_current_top_level_item()
lib/Conifer/Post/HasCustomAdminColumns.php Loosened types; fn syntax replaced with closures
lib/Conifer/Post/HasCustomAdminFilters.php Loosened types; fn syntax replaced with closures
lib/Conifer/Post/SupportsAdvancedSearch.php Loosened types; fn syntax replaced with closures
lib/Conifer/Authorization/*.php Loosened types; fn syntax replaced with closures
lib/Conifer/Twig/*.php Loosened types; first-class callables replaced with [$this, 'method'] arrays
lib/Conifer/Notifier/*.php Loosened types
lib/Conifer/Shortcode/*.php Loosened types
lib/Conifer/Navigation/*.php Loosened types
lib/Conifer/Integrations/YoastIntegration.php Loosened types
lib/Conifer/AjaxHandler/AbstractBase.php Loosened types
test/unit/** Removed declare(strict_types=1), void return types, and typed properties
test/integration/** Same loosening; setUp() lost : void return type
test/bootstrap.php Added setAccessible(true) calls required for PHP 8.1+
test/support/Person.php Loosened types
composer.json Removed Rector, downgraded wp-coding-standards/wpcs from ^3.2 to ^2.3, updated other deps
phpcs.xml Changed tab-width from 4 to 2; changed optionated (misspelling introduced); removed exact indent enforcement
package.json Bumped vitepress/vue; removed docs:dev and docs:preview scripts
.lando.yml Removed rector tooling; node version 22→24; build ordering changes
docs/** Migrated from GitBook to VitePress; updated governance, changelog, twig-helpers docs
.gitignore Cleaned up entries
.editorconfig Removed entirely

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +49 to +51
// The filter shouldn't return anything, so if it does we know something broke.
$this->assertNull($subPage->do_add());
}
Copy link

Copilot AI Mar 2, 2026

Choose a reason for hiding this comment

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

The AdminPageTest::test_do_add() and AdminSubPageTest::test_do_add_sub_page() tests were changed from asserting the fluent interface ($this->assertEquals($this->page, $this->page->do_add())) to asserting null ($this->assertNull($this->page->do_add())). The do_add() method now returns void, which is a breaking API change for anyone relying on the fluent interface of do_add(). The test comment "The filter shouldn't return anything, so if it does we know something broke" is misleading — this is a deliberate return type change, not a side effect check.

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@copilot code review[agent] Yes, the return type of those do_add() functions changed from Page to void, but the functions are only used in a WordPress filter like so: add_action('admin_menu', [$this, 'do_add']);. The admin_menu filter doesn't expect to be passed a Page object. It expects to be passed nothing, so it's been silently discarding it all this time.

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.

2 participants