Smart Links: Change data structure to use Status taxonomy#3213
Smart Links: Change data structure to use Status taxonomy#3213vaurdan merged 12 commits intoadd/traffic-boostfrom
Conversation
# Conflicts: # build/content-helper/editor-sidebar.asset.php # build/content-helper/editor-sidebar.js
vaurdan
added
Component: PHP
Refactoring
📝 WalkthroughWalkthroughThe changes refactor how smart link status is managed across multiple parts of the system. Direct property checks and assignments related to the applied state are replaced by method calls (such as Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API as "Endpoint_Smart_Linking"
participant SL as "Smart_Link Object"
participant DB as "Database"
Client->>API: Request to add/update smart link
API->>SL: Call set_status(APPLIED)
SL->>DB: Update status via taxonomy
DB-->>SL: Confirm update
SL-->>API: Return updated link
API-->>Client: Respond with success
sequenceDiagram
participant Client
participant TBoost as "Endpoint_Traffic_Boost"
participant Inbound as "Inbound_Smart_Link"
Client->>TBoost: Request link suggestions
TBoost->>Inbound: Call generate_link_suggestions (set status to PENDING)
Inbound-->>TBoost: Return suggestion with pending status
TBoost-->>Client: Deliver suggestions
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{html,php}`: "Perform a detailed review of the provided code with following key aspects in mind: - Review the HTML and PHP code to ensure it is well-structured and adheres ...
🧬 Code Definitions (1)wp-parsely.php (2)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🔇 Additional comments (6)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
# Conflicts: # build/content-helper/editor-sidebar.asset.php # build/content-helper/editor-sidebar.js # src/Models/class-smart-link.php
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
wp-parsely.php (1)
232-319: Added data schema update mechanism.This well-implemented function handles the migration of smart links from the old meta-based status system to the new taxonomy-based approach. Key strengths:
- Proper version checking to ensure the function runs only once
- Efficient query to find affected smart links
- Early return if no updates are needed
- Careful handling of existing meta data for backward compatibility
- Cache flushing to ensure fresh data after updates
- Cleanup of deprecated meta values
The function is well-documented with clear explanations of its purpose and logic.
However, note that
get_postswithposts_per_page = -1could potentially cause memory issues on sites with a very large number of smart links. Consider adding pagination for extremely large sites.- $smart_links_without_status = get_posts( - array( - 'post_type' => 'parsely_smart_link', - 'posts_per_page' => -1, - 'fields' => 'ids', - 'tax_query' => array( - array( - 'taxonomy' => 'smart_link_status', - 'field' => 'name', - 'terms' => \Parsely\Models\Smart_Link_Status::get_all_statuses(), - 'operator' => 'NOT IN', - ), - ), - ) - ); + $batch_size = 100; + $page = 1; + $smart_links_without_status = array(); + + do { + $batch = get_posts( + array( + 'post_type' => 'parsely_smart_link', + 'posts_per_page' => $batch_size, + 'paged' => $page, + 'fields' => 'ids', + 'tax_query' => array( + array( + 'taxonomy' => 'smart_link_status', + 'field' => 'name', + 'terms' => \Parsely\Models\Smart_Link_Status::get_all_statuses(), + 'operator' => 'NOT IN', + ), + ), + ) + ); + + $smart_links_without_status = array_merge($smart_links_without_status, $batch); + $page++; + } while (count($batch) === $batch_size);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
build/content-helper/editor-sidebar.asset.phpis excluded by!build/**build/content-helper/editor-sidebar.jsis excluded by!build/**
📒 Files selected for processing (6)
src/Models/class-inbound-smart-link.php(7 hunks)src/Models/class-smart-link.php(13 hunks)src/content-helper/editor-sidebar/smart-linking/provider.ts(2 hunks)src/content-helper/editor-sidebar/smart-linking/utils.ts(1 hunks)src/rest-api/content-helper/class-endpoint-smart-linking.php(3 hunks)wp-parsely.php(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- src/content-helper/editor-sidebar/smart-linking/utils.ts
- src/content-helper/editor-sidebar/smart-linking/provider.ts
- src/rest-api/content-helper/class-endpoint-smart-linking.php
- src/Models/class-inbound-smart-link.php
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.{html,php}`: "Perform a detailed review of the provided code with following key aspects in mind: - Review the HTML and PHP code to ensure it is well-structured and adheres ...
**/*.{html,php}: "Perform a detailed review of the provided code with following key aspects in mind:
- Review the HTML and PHP code to ensure it is well-structured and adheres to best practices.
- Ensure the code follows WordPress coding standards and is well-documented.
- Confirm the code is secure and free from vulnerabilities.
- Optimize the code for performance, removing any unnecessary elements.
- Validate comments for accuracy, currency, and adherence to WordPress coding standards.
- Ensure each line comment concludes with a period.
- Verify code compatibility with the latest version of WordPress, avoiding deprecated functions or features."
wp-parsely.phpsrc/Models/class-smart-link.php
🧬 Code Definitions (1)
wp-parsely.php (3)
src/class-parsely.php (1) (1)
Parsely(75-1152)src/Models/class-smart-link.php (4) (4)
Smart_Link(25-1078)get_smart_link_by_id(816-824)set_status(518-528)flush_all_cache(1032-1040)src/Models/class-inbound-smart-link.php (1) (1)
get_smart_link_by_id(1146-1153)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: E2E against WordPress latest
🔇 Additional comments (15)
src/Models/class-smart-link.php (14)
115-121: Property type change to enhance status management.This change correctly updates the property type from a boolean
appliedto a more flexiblestring|nullstatus, which allows for more granular state representation beyond binary applied/not-applied states.
251-257: Status property initialization added during object loading.The code now properly initializes the
statusproperty using theget_statusmethod when loading a smart link from the database, ensuring consistency with the persisted state.
371-376: Status persistence logic added to save method.This is a well-structured implementation for saving the smart link status to the database using WordPress taxonomy terms. The code properly validates the status before saving and falls back to a default
PENDINGstatus when needed.
402-402: Status reset in delete method.The status is correctly reset to null when deleting the smart link, maintaining consistency with the object's state.
468-496: New method to retrieve smart link status.This is a well-implemented method that:
- Returns the cached status if valid
- Falls back to retrieving from database
- Handles error conditions properly
- Returns a default
PENDINGstatus when neededThe code adheres to WordPress coding standards and includes proper documentation.
498-507: Added is_applied helper method.This method provides a clean abstraction for checking if a smart link is applied, improving readability throughout the codebase and centralizing status checking logic.
509-528: Added status setter with validation.Good implementation of a setter method with proper validation. The method:
- Validates the status value
- Optionally persists to database
- Updates the instance property
- Throws appropriate exceptions for invalid states
The parameter type hints and return type declarations are correctly used.
739-741: Updated array serialization to include status information.The serialization now includes both the raw status string and the boolean
appliedproperty for backward compatibility, which is a good practice during API transitions.
816-816: Changed visibility of get_smart_link_by_id method from protected to public.The method visibility has been changed to public to allow external access from the data schema update function. This change follows the principle of minimal permissions needed for the code to work properly.
838-843: Added status validation to get_outbound_smart_links method.The method now validates the provided status parameter and defaults to 'all' with an appropriate developer warning if an invalid status is provided.
853-876: Enhanced tax query for outbound smart links.The tax query has been refactored to properly filter by status, allowing for more efficient querying of smart links based on their current state.
935-956: Enhanced tax query for inbound smart links.Similar to the outbound links, this implementation properly structures the taxonomy query to filter by status when needed.
980-992: Added backward compatibility handling for link status.This code properly handles the transition from the old boolean-based system to the new status taxonomy by:
- Checking if a link is physically linked in the content
- Updating its status to match the actual state
- Ensuring data consistency
This is important for maintaining functionality during the transition.
1032-1032: Changed visibility of flush_all_cache method from protected to public.This change allows external components to flush the cache when needed, particularly during the data schema update process. The method's implementation remains unchanged.
wp-parsely.php (1)
52-54: Constants formatting alignment and addition of schema version.The constants are now properly aligned for better readability, and a new
PARSELY_DATA_SCHEMA_VERSIONconstant has been added to track data schema changes.
acicovic
left a comment
acicovic
left a comment
There was a problem hiding this comment.
Thanks for working on this. I've left a couple of minor comments, but also have 2 questions:
- Did you think to rely on plugin version for the update procedure, and if yes, what made you go with a new option instead?
- We should think the scenario where the plugin is downgraded. What are your thoughts regarding that?
|
@acicovic to answer your questions:
That was my initial plan, indeed. However, relying on the plugin version can be tricky in situations where the plugin is downloaded directly through GitHub, for example, where we only bump the version at release. This could be problematic for 3.18.0 beta since it actual version still is 3.17.0, so the schema update would not trigger. And if we need to change the schema in between development builds, this can also become a problem. That's why I added that new schema version option, so we can track the data layer version separately from the plugin version.
That's a pretty good question, and TBH I haven't considered that scenario. Right now, it isn't really possible, as AFAIK, it's not possible to trigger specific downgrade steps, since those steps would have need to be defined in the version where it is being downgraded to. Right now, from my testing (I had to update and downgrade a bunch of times to make sure this was working), what might happen when downgrading is that we lose the data if links have been applied. This means that the queries often return applied smart links as suggestions, and the UI can glitch out here and there. Not great. I think an easy way to make it backwards compatible, in case of a downgrade, is to not remove any of the existing |
|
Thanks for explaining about the schema, this makes sense and it's more flexible if we happen to need it. Regarding the downgrade path, if someone was to apply Smart Links in v3.18 and then go back to v3.17, would this also work for these Smart Links, or only for those who were applied before the upgrade? |
|
If someone downgrades from this version to 3.17.0, I think the only side effect is that all the existing smart links - even pending ones (suggestions) would be treated as applied, since the meta doesn't yet exists on that version, and each Smart Link in the database is actually an applied Smart Link. However, I'm pretty sure this would be auto-fixed by the Smart Linking feature, as each time there's a save, it parses all the existing Smart Links in the current article, saves those and discards the remaining. So those "applied" - but actually pending - smart links would get removed from the database without the user even realizing. I will change the PR so it doesn't remove the |
|
Sounds good, we can probably remove the meta tag in the next major/breaking release and flag it as breaking. BTW, an alternative approach would be to have a 3.17.1 patch, which executes the inverse procedure (if that is possible). But I think that just keeping the meta is better in this case. The reason for this discussion is that I think that breaking or non-reversible operations that could bring breakage upon downgrade warrant a major/breaking release. But happy to discuss this further if we want. |
2 participants
Description
This PR changes the data structure of the Smart Link, to use a new taxonomy - Smart Link Status - to store the status of a given smart link - either applied or pending.
This is considerable more performant than relying on post meta, since we need to do queries using the smart link status (get all inbound that are pending, get all outbound that are applied, etc).
I have also introduced a function in
wp-parsely.phpthat handles data schema updates. In this case it is necessary to prevent breaking changes for a couple reasons:_smart_link_appliedpost meta to applied smart links. Therefore, all the smart links that are added to the database at this version are considered applied;_smart_link_appliedto detect if the link is applied (trueif so, or pending iffalse).This update callback checks for Smart Links that do not have the
_smart_link_appliedpost meta, and if not, add theacceptedterm. If it does have the post meta, it will add the term depending on the meta value.The callback only runs once, and only when
PARSELY_DATA_SCHEMA_VERSIONis higher than the current version stored in theparsely_data_schema_versionoption.Motivation and context
How has this been tested?
Tested locally in a few different scenarios:
The existing tests also pass.
Summary by CodeRabbit
New Features
Refactor
Chores