Skip to content

Removed assets referred to from deprecated functions. #31395

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.

Sign up for GitHub

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

Jump to bottom
Merged
peterfabian merged 1 commit into from
Jan 12, 2022
Merged

Removed assets referred to from deprecated functions. #31395

peterfabian merged 1 commit into from
Jan 12, 2022

Conversation

@peterfabian
Copy link
Contributor

@peterfabian peterfabian commented Dec 8, 2021 •
edited
Loading

All Submissions:

Changes proposed in this Pull Request:

Removes images that are referred to from deprecated functions and methods. Not entirely backward compatible, but hoping not many people are actually using these in their plugins.

How to test the changes in this Pull Request:

  1. Check that these files are not referred from non-deprecated code.

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?
  • Have you successfully run tests with your changes locally?

Changelog entry

Removed images referred to from deprecated functions.

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.

@peterfabian peterfabian requested review from a team and barryhughes and removed request for a team December 8, 2021 15:53
@barryhughes
Copy link
Member

barryhughes commented Dec 8, 2021 •
edited
Loading

plugins/woocommerce/assets/images/jetpack_horizontal_logo.png

Using this image as an example, it's used from within wc_setup_activate(). I realize that function is deprecated, but suppose on some site for some reason it's still in use: won't the output look rather broken (and so shouldn't we also strip out the corresponding image markup)?

@barryhughes
Copy link
Member

barryhughes commented Dec 8, 2021

🤔 I'm thinking this isn't the focus of the PR and perhaps the goal is just to reduce package size; but building on my last thought it makes me wonder if we shouldn't just turn functions like wc_setup_activate() into noops.

barryhughes
barryhughes approved these changes Dec 8, 2021
View reviewed changes
Copy link
Member

@barryhughes barryhughes left a comment

Choose a reason for hiding this comment

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

Left a question, really just to double-check comfort with this change. Beyond that, LGTM (so marking as approved to expedite things if we're happy).

@peterfabian
Copy link
Contributor Author

peterfabian commented Dec 8, 2021 •
edited
Loading

Good question. Not sure at this point. We should probably form a policy about what to do with deprecated functions.

In this particular case, though, I think the whole old setup wizard is now deprecated and I'm not even sure if it can be accessed somehow, as on a new installation, the new OBW is started right after activation. We should just remove the whole file at some point, or turn all functions in there to noops. Let's discuss with the team.

@peterfabian peterfabian merged commit 38931e4 into trunk Jan 12, 2022
@peterfabian peterfabian deleted the fix/remove-old-images branch January 12, 2022 15:21
@github-actions github-actions bot added this to the 6.2.0 milestone Jan 12, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Jan 12, 2022 •
edited by peterfabian
Loading

Hi @peterfabian, thanks for merging this pull request. Please take a look at these follow-up tasks you may need to perform:

  • Add the status: needs changelog label
  • Add the status: needs testing instructions label

@peterfabian peterfabian added the release: add changelog Mark all PRs that have not had their changelog entries added. [auto] label Jan 12, 2022
@kloon
Copy link
Contributor

kloon commented Sep 12, 2022

@peterfabian I just spotted that this PR removed the storefront.png file while trunk is still referencing the file at

woocommerce/plugins/woocommerce/includes/admin/views/html-admin-page-addons.php

Line 142 in 5cba6b6

<a href="<?php echo esc_url( $storefront_url ); ?>" target="_blank"><img src="<?php echo esc_url( WC()->plugin_url() ); ?>/assets/images/storefront.png" alt="<?php esc_attr_e( 'Storefront', 'woocommerce' ); ?>" /></a>

Do you want me to open a new issue to restore the file?

@perceptions perceptions mentioned this pull request Dec 4, 2023
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@barryhughes barryhughes barryhughes approved these changes

Assignees

No one assigned

Labels

release: add changelog Mark all PRs that have not had their changelog entries added. [auto]

Projects

None yet

Milestone

6.2.0

Development

Successfully merging this pull request may close these issues.

4 participants

@peterfabian @barryhughes @kloon