-
Notifications
You must be signed in to change notification settings - Fork 1
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
Feature: Installer Script #46
Conversation
Note Reviews PausedUse the following commands to manage reviews:
WalkthroughThe project has undergone several key updates, including the introduction of a new rule for naming conventions of global variables, the addition of a "setup" target in the Makefile for project configuration, restructuring of the README.md to provide clearer instructions and new sections for project structure and maintenance, and the inclusion of an interactive script, Changes
Assessment against linked issues (Beta)
TipsChat with CodeRabbit Bot (
|
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.
Review Status
Actionable comments generated: 1
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- composer.json
Files selected for processing (6)
- .phpcs.xml (1 hunks)
- Makefile (1 hunks)
- README.md (1 hunks)
- configure.php (1 hunks)
- mu-plugins/000-wp-environment.php (1 hunks)
- mu-plugins/001-composer.php (2 hunks)
Files skipped from review due to trivial changes (4)
- .phpcs.xml
- Makefile
- README.md
- mu-plugins/001-composer.php
Additional comments: 1
mu-plugins/000-wp-environment.php (1)
- 2-33: The conditional check around the definition of
WP_ENVIRONMENT_TYPE
is a good practice to avoid redefining constants, which would cause a PHP error. The use of thematch
expression (introduced in PHP 8.0) is a modern and clean way to handle the mapping of environment names toWP_ENVIRONMENT_TYPE
values. However, ensure that the server environment is running PHP 8.0 or higher, as this code will not work on earlier versions.The unsetting of
$environment_source
is a good practice to avoid polluting the global namespace, especially since this script seems to be included in a WordPress mu-plugin, which is executed in the global context.
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.
Review Status
Actionable comments generated: 16
Configuration used: CodeRabbit UI
Files selected for processing (1)
- configure.php (1 hunks)
Additional comments: 8
configure.php (8)
99-102: The
run
function usesshell_exec
, which can be a security risk if not properly handled. Ensure that any dynamic input to this function is properly escaped to prevent command injection.
[SECURITY]191-258: The script collects various project details from the user or from the system's git configuration. This is a good use of default values and user input. However, ensure that the collected data is properly sanitized before being used in file operations to prevent security issues.
315-330: The search and replace array is a good way to handle bulk replacements. However, ensure that the keys are unique and won't accidentally replace unintended parts of the files.
358-362: The
composer config
command is being run with a JSON-encoded string. Ensure that the JSON encoding does not introduce any issues with shell execution and that the namespaces and paths are correctly formatted.366-375: The script scaffolds a plugin using Composer. Ensure that the Composer project being created is from a trusted source and that any post-create-project scripts are safe to run.
[SECURITY]412-418: The script modifies the
.github/CODEOWNERS
file. Ensure that the replacements don't accidentally remove necessary content and that the file still follows the expected format after modification.517-524: The self-deletion feature is a good cleanup step. However, ensure that the script can't accidentally delete itself in case of a wrong confirmation or path issue.
[SECURITY]1-528: Overall, the script is well-structured and covers a lot of functionality. Ensure that all external system calls are secure and consider adding more error handling and input validation throughout the script. Also, consider using more of PHP's built-in functions for file and directory operations for better portability and security.
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.
Left some suggestions, no blockers. Thanks for knocking this one out! 🍣
@srtfisher I don't think this addresses #23 as written - it does contain logic to configure the plugin based on target deployment type, but doesn't make it polymorphic. What @mogmarsh was suggesting in #23 was to make it so that the same codebase could be deployed to both VIP and Pantheon (e.g., by making the mu-plugins directory load stuff in client-mu-plugins, standardizing on client-mu-plugins even for Pantheon, etc). I'd recommend removing #23 from the list of issues that this PR addresses so that we can discuss further. |
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.
@coderabbitai resolve |
@coderabbitai pause |
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.
@coderabbitai resolve |
Adds the basis of an installer script that will scaffold the project as well as the project's main theme/plugin. It will search and replace wherever necessary and also configure the project for WordPress VIP/Pantheon as well as Buddy/GitHub Actions. It's ready for Buddy whenever #10 is done.
This PR leaves one TODO for scaffolding out the theme itself from
create-wordpress-theme
. Once that project is ready, we can publish it to Packagist and then uncomment the code here:create-wordpress-project/configure.php
Lines 378 to 386 in 8578432
Resolves #7, #4.
Food for thought: what should the namespace be for the theme/plugin? Should it be
Base_Namespace\Theme
orBase_Namespace_Theme
?Summary by CodeRabbit
Summary of changes
WP_ENVIRONMENT_TYPE
only if not already defined.