Update/build tooling etc - #122
Open
georgestephanis wants to merge 7 commits into
Open
Conversation
Bump minimum PHP to 7.2 and update dev dependencies (phpunit to ^8.5 || ^9.6, phpcs installer, phpcompatibility, and WPCS). Add Composer config.platform.php = 7.2.34 for consistent environment and enable allow-plugins for the phpcs installer plugin.
Adjust PHPCS XML configuration: remove the WordPress.CodeAnalysis.EmptyStatement rule; normalize the WordPress.WP.I18n text_domain property to use an <element> format; rename the customPropertiesWhitelist property to allowed_custom_properties to match the expected sniff name; and replace the WordPress.CodeAnalysis.AssignmentInCondition.FoundInWhileCondition rule with Generic.CodeAnalysis.AssignmentInCondition (keeping the exclude-pattern). These changes align the config with updated sniff expectations and simplify assignment-in-condition handling.
Replace dirname(__FILE__) with __DIR__ and update nested dirname usage to dirname(__DIR__, 2); standardize object instantiation by adding parentheses for new Class() and instantiate OAuthSignatureMethod_HMAC_SHA1 with constructor calls across OAuth1 services. Also include minor whitespace cleanup, adjust anonymous function spacing, and normalize debug string quotes across admin-ui.php, keyring.php, service.php, store.php and multiple includes/services files.
Convert numerous class properties from var to public and update function declarations to explicit public (and public static where applicable) across the admin UI, core OAuth/HTTP services, and many extended service implementations. This modernizes visibility declarations, clarifies intent and improves compatibility with newer PHP versions. Affected areas include admin-ui.php, includes/services/core/*, includes/services/extended/*, and several core store/token/service files; these are intended as stylistic/compatibility changes with no behavioral modifications.
Extract Keyring_Util and Keyring_Error into keyring-util.php and keyring-error.php; move Keyring_Request_Token and Keyring_Access_Token into token-request.php and token-access.php. Also extract Keyring_Connections_List_Table to admin-connections-list-table.php and update admin-ui.php and token.php to require the new files. This modularizes the codebase by separating utility, error, token types, and admin list-table logic into their own files and removes duplicate class definitions from larger files.
Two targeted changes:
Added protected $endpoints = array() to Keyring_Service — a backing store for custom endpoint types that individual services define (e.g. self, profile, notifications).
Updated set_endpoint() to branch on property_exists():
If the {type}_url property is already declared (e.g. authorize_url, access_token_url in Keyring_Service_OAuth1) → writes directly to it as before.
If the property is not declared (e.g. a service's custom self_url) → stores it in $this->endpoints['self'] instead of creating a dynamic property.
Added __get() magic method so that $this->self_url and $this->self_method continue to work transparently, reading from the $endpoints array — fully backward-compatible with all 27 service implementations, no changes needed elsewhere.
This approach works on PHP 7.2+ (no attributes required) and eliminates all dynamic property creation for custom endpoint types.
Author
|
Not sure how many of these tidies had already been made upstream on wpcom and not ported back down |
Expose a public callback_url property on OAuthConsumer (default null) and introduce a protected redirect_uri property on Keyring_Service_OAuth2 (default empty) with a docblock noting it should be set in service constructors. Changes update includes/oauth-php/OAuth.php and includes/services/core/oauth2.php to support OAuth callback/redirect handling.
Author
|
There wasn't a lot of creative input involved here, so the changes were largely Agentic driven, then I cycled around and tested manually to isolate any issues. |
pablinos
reviewed
Mar 18, 2026
pablinos
left a comment
pablinos
left a comment
Collaborator
There was a problem hiding this comment.
This looks reasonable to me. It's worth testing it against https://github.com/beaulebens/keyring-social-importers to make sure it doesn't break anything, but the public changes seem harmless, and it's mainly the dynamic property change. That looks fine also, but I added a minor comment.
|
|
||
| class Keyring_Util { | ||
| static function debug( $str, $level = KEYRING__DEBUG_NOTICE ) { | ||
| if ( ! KEYRING__DEBUG_MODE ) { |
Collaborator
There was a problem hiding this comment.
What do you think about these requires being at the top of the file?
| // Declared property (e.g. authorize_url, access_token_url in Keyring_Service_OAuth1). | ||
| // Write directly to avoid triggering __get/__set on a property that already exists. | ||
| $this->{$type . '_url'} = $url; | ||
| $this->{$type . '_method'} = strtoupper( $method ); |
Collaborator
There was a problem hiding this comment.
If the _url property is defined, does that mean the _method property is guaranteed? It's probably fine, because they'd be doing it wrong, and it would raise an error/warning, but if somone called this with callback as the type, then we'd try and create the _method property.
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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 pull request primarily refactors the codebase to modernize PHP syntax, improve maintainability, and separate concerns. The most significant changes include updating method and property visibility to
public, raising the minimum PHP version requirement, updating development dependencies, and moving theKeyring_Connections_List_Tableclass to its own file for better organization.Code modernization and PHP compatibility:
Updated all class properties and methods in files such as
admin-ui.php,includes/services/core/http-basic.php,includes/services/core/oauth1.php,includes/services/core/oauth2.php, andincludes/services/extended/0-google-base.phpto use explicitpublicvisibility, replacing the oldervarand default visibility. This brings the codebase in line with modern PHP standards. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15] [16] [17] [18] [19] [20]Increased the minimum required PHP version from 5.6 to 7.2 in
composer.json, and updated development dependencies (such as PHPUnit and coding standards) to versions compatible with PHP 7.2+. Added Composer config to ensure consistent PHP versioning and plugin allowances.Code organization and maintainability:
Keyring_Connections_List_Tableclass fromadmin-ui.phpinto a new fileadmin-connections-list-table.phpto improve separation of concerns and maintainability. The original class definition was removed fromadmin-ui.phpand replaced with arequire_oncestatement. [1] [2]These changes collectively improve code clarity, compatibility with newer PHP versions, and make the codebase easier to maintain and extend.