-
Notifications
You must be signed in to change notification settings - Fork 60
EPS-1483: New Post duplicator feature #1401
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: release-candidate
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,27 @@ | |||
| /** | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: Consider specifying a version for the CSS file if applicable.
Why: Versioning helps in managing assets and ensures that users receive the correct style updates. It aids in cache management.
How: Add a comment at the top of the CSS file indicating the version, e.g., /* Version 1.0.0 */. This practice can be integrated with your build process or asset management approach.
| @@ -0,0 +1,27 @@ | |||
| /** | |||
| * Admin styles for Post Duplicator. | |||
| * | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: Consider adding more context to the comment above the .hfe-post-duplicator-notice class.
Why: While the comment generally states it's for the Post Duplicator, including what the notice signifies (e.g., success or error messages) can improve maintainability and understanding for future developers.
How: Change the comment to something like: /* Styles for success/error notices related to the Post Duplicator feature. */.
| /** | ||
| * Admin styles for Post Duplicator. | ||
| * | ||
| * @package header-footer-elementor |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: The color definitions used here may benefit from being stored as CSS variables for easier theming or future changes.
Why: Utilizing CSS variables enhances maintainability and makes it easier to apply changes if the color scheme requires updates later on.
How: Define CSS variables in a root selector, like :root { --primary-color: #0073aa; --hover-color: #00a0d2; }, then use these variables in your styles, e.g., color: var(--primary-color);.
| public function includes() { | ||
| require_once HFE_DIR . 'admin/class-hfe-admin.php'; | ||
|
|
||
| require_once HFE_DIR . 'inc/hfe-functions.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: The addition of the Post Duplicator class appears to be correctly included, but I recommend considering the implications of loading this class for all users who access the admin interface. If this class introduces heavy operations or data processing, it could impact performance.
Why: Loading unnecessary classes can lead to increased memory usage and slower performance, particularly for users who do not utilize the duplication feature. This is crucial for maintaining a responsive admin experience, especially if many users will be accessing the admin interface.
How: To improve performance, consider utilizing a conditional loading mechanism. For example, load the class-hfe-post-duplicator.php file only if the user is accessing a specific admin page or if the feature is enabled in the settings. This can be implemented using a check like if (isset($_GET['page']) && $_GET['page'] === 'desired_page_name'). This reduces unnecessary overhead for users who do not need the duplication functionality.
| // Create duplicate link. | ||
| $duplicate_link = admin_url( 'admin.php?action=hfe_duplicate_post&post=' . $post->ID . '&nonce=' . $nonce ); | ||
|
|
||
| // Add duplicate link to actions. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: Sanitization for retrieved GET parameters is insufficient.
Why: While the code uses absint() for sanitizing the post parameter, it directly uses the nonces without appropriate sanitization for the nonce value being passed in the GET request. This could lead to security loopholes if it were to validate an invalid or malicious nonce.
How: Instead of using sanitize_text_field after wp_unslash, use sanitize_key() for the nonce variable. Ensure that nonce validation occurs immediately after any necessary sanitization.
|
|
||
| /** | ||
| * Duplicate post. | ||
| * |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: Potential performance concern with the copy_post_meta function; unnecessary iterations.
Why: The code retrieves all post meta and updates each key/value pair one by one. If there are many meta fields, this could lead to performance issues because of the multiple database updates.
How: Consider using a bulk insert operation where possible, or if there's a way to process multiple key/value pairs efficiently that may reduce the number of database operations.
| 'post_author' => $post->post_author, | ||
| 'post_content' => $post->post_content, | ||
| 'post_excerpt' => $post->post_excerpt, | ||
| 'post_parent' => $post->post_parent, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What: Enhance error handling when duplicating post; currently generic.
Why: When creating a duplicate post, the user receives a generic message if an error occurs, without knowing the actual issue that caused the failure (such as 'invalid data provided', etc.). This could lead to confusion in usage.
How: Instead of directly passing the error messages from wp_error, consider logging the error for development purposes and providing a user-friendly explanation to the admin that provides information on what the issue could be.
Description
Main Purpose: This pull request introduces a new feature for duplicating posts within the admin interface of the Header Footer Elementor plugin. This functionality allows users to create copies of existing posts, enhancing content management for users who need to replicate posts quickly.
Key Changes:
HFE_Post_Duplicatorhas been created inclass-hfe-post-duplicator.php, which handles the duplication functionality.admin_postactions have been implemented to manage post duplication requests and enqueue admin-specific CSS styles fromadmin-post-duplicator.css.add_post_type_filtersmethod dynamically adds a "UA Duplicate" link to the row actions for each public post type in the admin interface.Additional Notes:
duplicate_post,create_duplicate, and the admin notice for successful duplication which enhances user experience.Screenshots
Types of changes
How has this been tested?
Checklist: