Skip to content
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

Update/composer install lib #35

Merged
merged 6 commits into from
Feb 23, 2022
Merged

Update/composer install lib #35

merged 6 commits into from
Feb 23, 2022

Conversation

darylldoyle
Copy link
Collaborator

Description of the Change

  • This moves the composer installed library to the root and makes sure that we're not including the entire library within the repo.
  • It also updates the build steps to run composer install rather than npm run build.

Checklist:

  • I have read the CONTRIBUTING document.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my change.
  • All new and existing tests passed.

Changelog Entry

  • Added - New build step
  • Changed - Library location

@darylldoyle darylldoyle self-assigned this Feb 22, 2022
@darylldoyle darylldoyle added the type:enhancement New feature or request. label Feb 22, 2022
@jeffpaul jeffpaul added this to the 1.9.10 milestone Feb 22, 2022
@jeffpaul
Copy link
Member

@dkotter this is probably worth a review to get into the 1.9.10 release to ensure things work smoothly as our first release under 10up / using our GitHub Actions.

@jeffpaul jeffpaul removed their request for review February 22, 2022 16:18
@@ -13,8 +13,7 @@ jobs:
uses: actions/checkout@v2
- name: Build
run: |
npm install
npm run build
composer install
Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks like we have a similar build command in the wordpress-plugin-asset-update.yml file. Should that be updated as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! I missed that other build command. That's been updated now!

safe-svg.php Outdated
@@ -14,7 +14,7 @@

defined( 'ABSPATH' ) or die( 'Really?' );

require 'lib/vendor/autoload.php';
require 'vendor/autoload.php';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since we aren't committing this file to the repo anymore, I think we need to add a conditional around this so those that are directly checking this repo out don't end up with a fatal error if they haven't run composer install.

Something like:

Suggested change
require 'vendor/autoload.php';
// Try and include our autoloader.
if ( is_readable( __DIR__ . '/vendor/autoload.php' ) ) {
require __DIR__ . '/vendor/autoload.php';
} else {
add_action( 'admin_notices', function() { ?>
<div class="notice notice-error">
<?php /* translators: %1$s: composer command */ ?>
<p><?php echo wp_kses_post( sprintf( __( 'You appear to be running a development version of Safe SVG. Please run %1$s in order for things to work properly.', 'safe-svg' ), '<code>composer install</code>' ) ); ?></p>
</div>
<?php
} );
return;
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call, I've added this in 🙂

@dkotter
Copy link
Collaborator

dkotter commented Feb 22, 2022

Should probably also add vendor/* into our .gitignore file

@darylldoyle
Copy link
Collaborator Author

@dkotter, thanks for taking a look. I've made those updates to the PR!

@dkotter dkotter merged commit b4825d2 into develop Feb 23, 2022
@dkotter dkotter deleted the update/composer-install-lib branch February 23, 2022 15:18
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:enhancement New feature or request.
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

4 participants