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

Move "composer/installers" package to require-dev. #1513

Merged
merged 4 commits into from
Sep 30, 2024
Merged

Conversation

mreishus
Copy link
Contributor

@mreishus mreishus commented Sep 18, 2024

Changes proposed in this Pull Request:

I found that wpcomsh was shipping with a large number of installer files in vendor and in the classmap:

2024-09-18_13-55

These don't look relevant to anything we need to do. Tracing it down, this dependency was added here when adding linting commands to the repo: #90

It might have also been added here instead: a9a44f4

As far as I can tell, composer/installers lets you install plugins to places other than vendor/, but I don't see it being used here. @anomiex also ran into some issues from it being included here: Automattic/jetpack#37737 (comment)

I think that means this is only needed for development (if at all), and we don't need to be including this when shipping the plugin. I searched for instances of "installer" and don't see it being used anywhere.

How to test the changes in this Pull Request:

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?

FOR PR REVIEWER ONLY:

  • I have reviewed that everything is sanitized/escaped appropriately for any SQL or XSS injection possibilities. I made sure Linting is not ignored or disabled.

Copy link

github-actions bot commented Sep 18, 2024

Size Change: 0 B

Total Size: 201 kB

ℹ️ View Unchanged
Filename Size
./build/53.js 1.08 kB
./build/index.css 731 B
./build/index.js 126 kB
./build/marketing.js 58.3 kB
./build/payment-gateway-suggestions.css 1.24 kB
./build/payment-gateway-suggestions.js 6.57 kB
./build/plugins.js 3.93 kB
./build/style-index.css 2.15 kB
./build/style-marketing.css 800 B

compressed-size-action

@mreishus mreishus self-assigned this Sep 18, 2024
@mreishus mreishus requested a review from joshuatf September 18, 2024 20:00
Copy link
Contributor

@ivan-ottinger ivan-ottinger left a comment

Choose a reason for hiding this comment

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

Hey, @mreishus. 👋🏼 I am probably not the most suitable person to judge the change proposed in this PR so will let it for others.

Thanks for the review request anyways!

@mreishus mreishus requested a review from chihsuan September 23, 2024 15:28
@obenland
Copy link
Member

@Automattic/somewherewarm Could you help with a review for this PR?

Copy link
Member

@chihsuan chihsuan left a comment

Choose a reason for hiding this comment

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

LGTM. 👍 I did a quick test on a E-commerce plan site and all seems to be working fine. Thanks for the PR. @mreishus

@PanosSynetos
Copy link
Contributor

Hey @obenland - SomewhereWarm hasn't worked with Calypso Bridge for over six months. Someone that is using Calypso Bridge on a daily basis, should be better suited to test this on out.

Copy link
Contributor

@ilyasfoo ilyasfoo left a comment

Choose a reason for hiding this comment

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

Thanks for reviewing, @chihsuan!

@mreishus Apologies to have missed this earlier. I've also tested this change in a business and a ecommerce plan site. I'm unsure how to fully test the change, but what I did was:

  1. In a fresh clone, run npm install, composer install, and npm run build and observed no related error message
  2. Upload the entire directory to wp-content/mu-plugins/wpcomsh/vendor/automattic/wc-calypso-bridge of the site
  3. Tested wp-admin and frontend and observed no error
  4. Looked at wp php-errors and observed no error
  5. Looked at wp-content/debug.log and observed no error

I've also updated the changelog in 23f91b7.

LGTM, feel free to merge. Let me know if you need help with deploying

@mreishus mreishus merged commit e1bbc46 into master Sep 30, 2024
5 checks passed
@mreishus
Copy link
Contributor Author

Thanks for the review! Created deploy PR #1526

@mreishus mreishus deleted the update/installers branch September 30, 2024 14:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants