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

fix: memory leaks from legacy term utilities #81

Merged
merged 4 commits into from
Jun 30, 2021
Merged

Conversation

dkoo
Copy link
Contributor

@dkoo dkoo commented Jun 23, 2021

All Submissions:

Changes proposed in this Pull Request:

Fixes and improvements coming from feedback from the v2 beta test. Related issues reported in #76.

How to test the changes in this Pull Request:

Legacy term conversion memory leak

In v2+ of Listings, listing CPTs use regular post categories and tags instead of separate custom taxonomies. We have a utility in place to check for and convert those legacy custom taxonomies, but it's an expensive operation. This PR avoids running the legacy term conversion script on every admin_init hook. Instead, moves the conversion to a WP CLI command so it has to be executed manually.

Addresses #76 (comment).

  1. On master, create some legacy Listing Categories and Listing Tags and assign to some listings.
  2. Check out this branch.
  3. Run wp newspack-listings taxonomies convert --dry-run while SSHed into your test site. Confirm that the script iterates through your legacy categories and tags and prints info on each, but doesn't persist any changes.
  4. Run wp newspack-listings taxonomies convert. This time confirm that the legacy terms are converted to regular post categories and tags, and that the converted post categories and tags are applied to the same listings that previously had the legacy terms.

Missing/orphan shadow term memory leak

Shadow terms are what we use to assign listings to other listings and posts/pages as related content. It's the most performant way to associate posts with other posts in WP. Unfortunately, terms sometimes go missing, or their posts get unpublished or deleted, resulting in orphaned terms. We have some utilities in place to check for and fix these missing/orphaned terms, but they are expensive operations. This PR avoids a memory leak caused by the functions that create missing or delete orphaned shadow terms by moving those to a WP CLI command.

  1. On this branch, publish some listings of any type, e.g. a Place. The plugin should create a shadow term for each place when it's published.
  2. Using WP CLI, look up the shadow terms using wp term list newspack_lstngs_plc. You should see a table of the shadow terms for your published places, e.g.:
+---------+--------------------+--------------------+-------------------------+-------------+--------+-------+
| term_id | term_taxonomy_id   | name               | slug                    | description | parent | count |
+---------+--------------------+--------------------+-------------------------+-------------+--------+-------+
| 488     | 488                | A New Place        | a-new-place             |             | 0      | 1     |
+---------+--------------------+--------------------+-------------------------+-------------+--------+-------+
  1. Delete one of the terms using wp term delete newspack_lstngs_plc <term_id>. This will result in a Place with a missing shadow term.
  2. Create an orphaned term by using wp term create newspack_lstngs_plc Orphan --description="An orphaned term.". This will result in a shadow term without a corresponding Place listing.
  3. Run wp newspack-listings taxonomies sync --dry-run. Confirm that the script identifies your missing and orphaned terms, but doesn't actually make any DB changes.
  4. Run wp newspack-listings taxonomies sync. This time confirm that the missing shadow term is created and the orphaned shadow term is deleted (using wp term list newspack_lstngs_plc again).

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 ran tests with your changes locally?

@dkoo dkoo changed the title feat: gate custom tax migration behind env flag to avoid expensive ops fix: fixes and improvements from v2 beta test Jun 23, 2021
@dkoo dkoo mentioned this pull request Jun 23, 2021
@dkoo dkoo changed the title fix: fixes and improvements from v2 beta test fix: memory leaks from legacy term utiltiies Jun 24, 2021
@dkoo dkoo changed the title fix: memory leaks from legacy term utiltiies fix: memory leaks from legacy term utiltities Jun 24, 2021
@dkoo dkoo changed the title fix: memory leaks from legacy term utiltities fix: memory leaks from legacy term utilities Jun 24, 2021
@dkoo dkoo added bug Something isn't working [Status] Needs Review labels Jun 24, 2021
@dkoo dkoo self-assigned this Jun 24, 2021
@dkoo dkoo added this to the Phase 2 milestone Jun 24, 2021
@dkoo dkoo marked this pull request as ready for review June 24, 2021 17:07
Copy link
Member

@adekbadek adekbadek left a comment

Choose a reason for hiding this comment

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

Works as described 🙌

@dkoo dkoo merged commit 4576805 into epic/phase-2 Jun 30, 2021
@dkoo dkoo deleted the fix/beta-test-fixes branch June 30, 2021 15:48
matticbot pushed a commit that referenced this pull request Jun 30, 2021
# [2.0.0-alpha.2](v2.0.0-alpha.1...v2.0.0-alpha.2) (2021-06-30)

### Bug Fixes

* default listings to one-column-wide.php post template ([#77](#77)) ([e987e76](e987e76))
* memory leaks from legacy term utilities ([#81](#81)) ([4576805](4576805))
matticbot pushed a commit that referenced this pull request Jun 30, 2021
# [2.0.0-alpha.2](v2.0.0-alpha.1...v2.0.0-alpha.2) (2021-06-30)

### Bug Fixes

* default listings to one-column-wide.php post template ([#77](#77)) ([e987e76](e987e76))
* memory leaks from legacy term utilities ([#81](#81)) ([4576805](4576805))
matticbot pushed a commit that referenced this pull request Jun 30, 2021
# [2.0.0-alpha.2](v2.0.0-alpha.1...v2.0.0-alpha.2) (2021-06-30)

### Bug Fixes

* default listings to one-column-wide.php post template ([#77](#77)) ([e987e76](e987e76))
* memory leaks from legacy term utilities ([#81](#81)) ([4576805](4576805))
matticbot pushed a commit that referenced this pull request Jun 30, 2021
# [2.0.0-alpha.2](v2.0.0-alpha.1...v2.0.0-alpha.2) (2021-06-30)

### Bug Fixes

* default listings to one-column-wide.php post template ([#77](#77)) ([e987e76](e987e76))
* memory leaks from legacy term utilities ([#81](#81)) ([4576805](4576805))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 2.0.0-alpha.2 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

dkoo added a commit that referenced this pull request Jul 6, 2021
* chore: replace AutocompleteTokenfield with Newspack Components version (#40)

* feat: use post categories and tags for all listing post types (#39)

* feat: use post categories and tags for all listing post types

* fix: undo patterns post type changes (for another PR)

* chore: remove unneeded admin page highlighting filter

BREAKING CHANGE: This feature will deprecate existing custom taxonomies, so any existing terms for those taxonomies will be lost.

To fix, we can convert terms from the deprecated taxonomies to standard post categories/tags via a migration script.


Fixes #32.

* feat: better integration with Newspack Theme features

* feat: add settings for individual listing type URL slugs

Closes #41.

* fix: wp_insert_post filter name and theme_mod filter

* fix: newspack_blocks support slug

* feat: add revisions support for listings

* chore: update outdated docblock

* refactor: use material icons with Newspack color

* fix: warning about default meta value

* fix: util for checking post type on new posts

* chore: update label of "hide author" setting

* feat: flush permalinks automatically if updating slug option

* feat: update cpt icon and block icons

See #49

* fix: remove material packages

* fix: failing npm ci command

* [WIP] feat: harden post type usage and establish relationships between listings and posts/pages (#43)

* feat: make Business patterns applicable only to places

* feat: create shadow taxonomy for places

* feat: add ability to associate Places with regular posts and pages

* fix: remove default map markers from patterns

* feat: new Price block for Marketplace listings

Also create a proof-of-concept Marketplace listing pattern to test block implementation.

* feat: add meta field with data synced from Price block

* feat: second test pattern

* fix: incorrect conditional for updating shadow terms (whoops)

* feat: abstract shadow tax functions to make it easier to add more

* refactor: simplify setting formattedPrice attribute via useEffect

* chore: fix PHP warning about passing variables

* feat: shadow taxonomies for all CPTs; test automated related listings

* fix: avoid showing post on itself

* fix: incorrect var assignment

* fix: decode HTML entities before rendering currency options

* chore: fix function/class docs

* fix: formatting an empty string results in NaN

* fix: wrong post type being returned by util

* fix: handle all listing post types for shadow taxonomies

* fix: don't show the shadow taxonomy for the post being edited

* feat: add new post button for shadow taxonomies

* chore: instantiate var as empty array, just in case

* fix: updating of terms, and delete orphan terms

* fix: replace Material icon with WordPress icon

* feat: custom UI for parent/child listings instead of WP taxonomy UI

* chore: update function comment

* chore: remove commented-out code

* fix: posts/pages child search fields; only show child UI if published

* fix: logical errors in validation method 🤦

* chore: update function name and comments for clarity

* refactor: use AutocompleteWithSuggestions from newspack-components (#56)

* feat: CSV importer script (#51)

* feat: init CSV importer via CLI command

* refactor: move importer files to subdir

* feat: take field mappings from config file instead of hard-coding them

* feat: add (currently unused) image_ids meta field for future use

* feat: process meta fields and content into block patterns

* fix: mismatch between data types in term handling

* feat: support multiple post types for importing

* docs: add README and sample config

* fix: do not apply content filters to legacy content upon importing

* chore: description

* fix: check if default post type constant is a listing

* feat: update block template; handle social media links

* feat: importer edits for GDG

* feat: add a new global setting and post option to hide date (#57)

* fix: use value property of selection from AutocompleteWithSuggestions (#61)

* fix: use value property of selection from AutocompleteWithSuggestions

* fix: cast listing ID value as an integer

* fix: cast listing IDs as strings to avoid breaking existing listings

* feat: convert legacy custom terms to regular post terms (#67)

* test: init test suite (#60)

* test: init test suite

* chore: fix function comment

* chore: remove redundant lint-php NPM script

* feat: if no view.php, register blocks with default attributes

* fix: activation PHP warning (#70)

* feat: support Newspack Sponsors for listings (#65)

* feat: support Newspack Sponsors

* refactor: response object with author and sponsors

* fix: sponsors for queried listings

* feat: update price block to use placeholder and large font size (#71)

* fix: guard against nonexistent meta object (#66)

* feat: child and related listings UI (#58)

* feat: initial exploration of UI in a modal

* fix: filter out parent post from child listings response

* feat: child and related listings UI

* chore: clear messages when closing modals

* chore: hide shadow terms from menu

* chore: handle errors from fetching suggestions

* chore: more context-sensitive labels for child/parent UI

* chore: update empty message

* refactor: make parent/child UI components more self-contained, reusable

* chore: update newspack-components

* fix: parent/child listings UI bugs

* chore: rename "Child Listings" to "Related Content"

* fix: default listings to one-column-wide.php post template (#77)

* chore: gate legacy taxonomy term deletion behind an environment constant

* fix: memory leaks from legacy term utilities (#81)

* feat: gate custom tax migration behind env flag to avoid expensive ops

* feat: move legacy taxonomy migrator script to a CLI command

* chore: rename CLI command and use ::log instead of ::line

* refactor: move missing/orphan shadow term handling to WP CLI

Co-authored-by: Thomas Guillot <info@thomasguillot.com>
Co-authored-by: Thomas Guillot <thomasguillot@users.noreply.github.com>
matticbot pushed a commit that referenced this pull request Jul 6, 2021
# [2.0.0-alpha.3](v2.0.0-alpha.2...v2.0.0-alpha.3) (2021-07-06)

* v2 release (#85) ([748810d](748810d)), closes [#85](#85) [#40](#40) [#39](#39) [#32](#32) [#41](#41) [#49](#49) [#43](#43) [#56](#56) [#51](#51) [#57](#57) [#61](#61) [#67](#67) [#60](#60) [#70](#70) [#65](#65) [#71](#71) [#66](#66) [#58](#58) [#77](#77) [#81](#81)

### Bug Fixes

* errors and bugs related to WP 5.8 ([#83](#83)) ([90da6c5](90da6c5))

### BREAKING CHANGES

* This feature will deprecate existing custom taxonomies, so any existing terms for those taxonomies will be lost.

To fix, we can convert terms from the deprecated taxonomies to standard post categories/tags via a migration script.
matticbot pushed a commit that referenced this pull request Jul 6, 2021
# [2.0.0](v1.2.2...v2.0.0) (2021-07-06)

* v2 release (#85) ([748810d](748810d)), closes [#85](#85) [#40](#40) [#39](#39) [#32](#32) [#41](#41) [#49](#49) [#43](#43) [#56](#56) [#51](#51) [#57](#57) [#61](#61) [#67](#67) [#60](#60) [#70](#70) [#65](#65) [#71](#71) [#66](#66) [#58](#58) [#77](#77) [#81](#81)

### Bug Fixes

* errors and bugs related to WP 5.8 ([#83](#83)) ([90da6c5](90da6c5))

### BREAKING CHANGES

* This feature will deprecate existing custom taxonomies, so any existing terms for those taxonomies will be lost.

To fix, we can convert terms from the deprecated taxonomies to standard post categories/tags via a migration script.
dkoo added a commit that referenced this pull request Jul 19, 2021
* chore: replace AutocompleteTokenfield with Newspack Components version (#40)

* feat: use post categories and tags for all listing post types (#39)

* feat: use post categories and tags for all listing post types

* fix: undo patterns post type changes (for another PR)

* chore: remove unneeded admin page highlighting filter

BREAKING CHANGE: This feature will deprecate existing custom taxonomies, so any existing terms for those taxonomies will be lost.

To fix, we can convert terms from the deprecated taxonomies to standard post categories/tags via a migration script.


Fixes #32.

* feat: better integration with Newspack Theme features

* feat: add settings for individual listing type URL slugs

Closes #41.

* fix: wp_insert_post filter name and theme_mod filter

* fix: newspack_blocks support slug

* feat: add revisions support for listings

* chore: update outdated docblock

* refactor: use material icons with Newspack color

* fix: warning about default meta value

* fix: util for checking post type on new posts

* chore: update label of "hide author" setting

* feat: flush permalinks automatically if updating slug option

* feat: update cpt icon and block icons

See #49

* fix: remove material packages

* fix: failing npm ci command

* [WIP] feat: harden post type usage and establish relationships between listings and posts/pages (#43)

* feat: make Business patterns applicable only to places

* feat: create shadow taxonomy for places

* feat: add ability to associate Places with regular posts and pages

* fix: remove default map markers from patterns

* feat: new Price block for Marketplace listings

Also create a proof-of-concept Marketplace listing pattern to test block implementation.

* feat: add meta field with data synced from Price block

* feat: second test pattern

* fix: incorrect conditional for updating shadow terms (whoops)

* feat: abstract shadow tax functions to make it easier to add more

* refactor: simplify setting formattedPrice attribute via useEffect

* chore: fix PHP warning about passing variables

* feat: shadow taxonomies for all CPTs; test automated related listings

* fix: avoid showing post on itself

* fix: incorrect var assignment

* fix: decode HTML entities before rendering currency options

* chore: fix function/class docs

* fix: formatting an empty string results in NaN

* fix: wrong post type being returned by util

* fix: handle all listing post types for shadow taxonomies

* fix: don't show the shadow taxonomy for the post being edited

* feat: add new post button for shadow taxonomies

* chore: instantiate var as empty array, just in case

* fix: updating of terms, and delete orphan terms

* fix: replace Material icon with WordPress icon

* feat: custom UI for parent/child listings instead of WP taxonomy UI

* chore: update function comment

* chore: remove commented-out code

* fix: posts/pages child search fields; only show child UI if published

* fix: logical errors in validation method 🤦

* chore: update function name and comments for clarity

* refactor: use AutocompleteWithSuggestions from newspack-components (#56)

* feat: CSV importer script (#51)

* feat: init CSV importer via CLI command

* refactor: move importer files to subdir

* feat: take field mappings from config file instead of hard-coding them

* feat: add (currently unused) image_ids meta field for future use

* feat: process meta fields and content into block patterns

* fix: mismatch between data types in term handling

* feat: support multiple post types for importing

* docs: add README and sample config

* fix: do not apply content filters to legacy content upon importing

* chore: description

* fix: check if default post type constant is a listing

* feat: update block template; handle social media links

* feat: importer edits for GDG

* feat: add a new global setting and post option to hide date (#57)

* fix: use value property of selection from AutocompleteWithSuggestions (#61)

* fix: use value property of selection from AutocompleteWithSuggestions

* fix: cast listing ID value as an integer

* fix: cast listing IDs as strings to avoid breaking existing listings

* feat: convert legacy custom terms to regular post terms (#67)

* test: init test suite (#60)

* test: init test suite

* chore: fix function comment

* chore: remove redundant lint-php NPM script

* feat: if no view.php, register blocks with default attributes

* fix: activation PHP warning (#70)

* feat: support Newspack Sponsors for listings (#65)

* feat: support Newspack Sponsors

* refactor: response object with author and sponsors

* fix: sponsors for queried listings

* feat: update price block to use placeholder and large font size (#71)

* fix: guard against nonexistent meta object (#66)

* feat: child and related listings UI (#58)

* feat: initial exploration of UI in a modal

* fix: filter out parent post from child listings response

* feat: child and related listings UI

* chore: clear messages when closing modals

* chore: hide shadow terms from menu

* chore: handle errors from fetching suggestions

* chore: more context-sensitive labels for child/parent UI

* chore: update empty message

* refactor: make parent/child UI components more self-contained, reusable

* chore: update newspack-components

* fix: parent/child listings UI bugs

* chore: rename "Child Listings" to "Related Content"

* fix: default listings to one-column-wide.php post template (#77)

* chore: gate legacy taxonomy term deletion behind an environment constant

* fix: memory leaks from legacy term utilities (#81)

* feat: gate custom tax migration behind env flag to avoid expensive ops

* feat: move legacy taxonomy migrator script to a CLI command

* chore: rename CLI command and use ::log instead of ::line

* refactor: move missing/orphan shadow term handling to WP CLI

* feat: block patterns for real estate and classifieds

* fix: import pattern styles into editor SCSS

* fix: restore patterns after merge

* refactor: remove placeholder image URLs from patterns

Co-authored-by: Thomas Guillot <info@thomasguillot.com>
Co-authored-by: Thomas Guillot <thomasguillot@users.noreply.github.com>
matticbot pushed a commit that referenced this pull request Jul 19, 2021
# [3.0.0-alpha.1](v2.0.1...v3.0.0-alpha.1) (2021-07-19)

### Bug Fixes

* avoid meta sync update error ([#95](#95)) ([cab16aa](cab16aa))
* do not register post-specific sidebars in widgets page ([#93](#93)) ([7716775](7716775))

### Features

* bump max number of items per list from 20 to 50 ([#97](#97)) ([009deab](009deab))
* more block patterns (real estate, classified ads) ([#84](#84)) ([e76acc3](e76acc3)), closes [#40](#40) [#39](#39) [#32](#32) [#41](#41) [#49](#49) [#43](#43) [#56](#56) [#51](#51) [#57](#57) [#61](#61) [#67](#67) [#60](#60) [#70](#70) [#65](#65) [#71](#71) [#66](#66) [#58](#58) [#77](#77) [#81](#81)

### BREAKING CHANGES

* This feature will deprecate existing custom taxonomies, so any existing terms for those taxonomies will be lost.

To fix, we can convert terms from the deprecated taxonomies to standard post categories/tags via a migration script.
matticbot pushed a commit that referenced this pull request Jul 19, 2021
# [3.0.0-alpha.1](v2.0.1...v3.0.0-alpha.1) (2021-07-19)

### Bug Fixes

* avoid meta sync update error ([#95](#95)) ([cab16aa](cab16aa))
* do not register post-specific sidebars in widgets page ([#93](#93)) ([7716775](7716775))

### Features

* bump max number of items per list from 20 to 50 ([#97](#97)) ([009deab](009deab))
* more block patterns (real estate, classified ads) ([#84](#84)) ([a51f5af](a51f5af))
* more block patterns (real estate, classified ads) ([#84](#84)) ([e76acc3](e76acc3)), closes [#40](#40) [#39](#39) [#32](#32) [#41](#41) [#49](#49) [#43](#43) [#56](#56) [#51](#51) [#57](#57) [#61](#61) [#67](#67) [#60](#60) [#70](#70) [#65](#65) [#71](#71) [#66](#66) [#58](#58) [#77](#77) [#81](#81)

### BREAKING CHANGES

* This feature will deprecate existing custom taxonomies, so any existing terms for those taxonomies will be lost.

To fix, we can convert terms from the deprecated taxonomies to standard post categories/tags via a migration script.
matticbot pushed a commit that referenced this pull request Jul 19, 2021
# 1.0.0-alpha.1 (2021-07-19)

### Bug Fixes

* avoid meta sync update error ([#95](#95)) ([cab16aa](cab16aa))
* do not register post-specific sidebars in widgets page ([#93](#93)) ([7716775](7716775))
* editor errors with reusable blocks ([#89](#89)) ([fdc46d3](fdc46d3))
* errors and bugs related to WP 5.8 ([#83](#83)) ([90da6c5](90da6c5))
* force alpha rebuild ([acc2075](acc2075))
* minor bug fixes ([#21](#21)) ([5f90bc7](5f90bc7))
* missing condition for block appender in list container ([#74](#74)) ([2c49896](2c49896))
* syncing attributes from curated list block to inner blocks ([#64](#64)) ([cdbc0bb](cdbc0bb))
* use synced attributes for ListContainer directly ([#73](#73)) ([f8641a7](f8641a7))

### Features

* add block patterns ([#23](#23)) ([a273a40](a273a40))
* bump max number of items per list from 20 to 50 ([#97](#97)) ([009deab](009deab))
* initial post type and block setup ([#1](#1)) ([47dc0c1](47dc0c1))
* listing taxonomies and query mode ([#6](#6)) ([528e1e5](528e1e5))
* more block patterns (real estate, classified ads) ([#84](#84)) ([a51f5af](a51f5af))
* new Curated List block, block pattern, and map functionality ([#3](#3)) ([9be6e7e](9be6e7e))
* remove borders and padding in editor to match front-end styles ([#14](#14)) ([6c47a17](6c47a17))

* v2 release (#85) ([748810d](748810d)), closes [#85](#85) [#40](#40) [#39](#39) [#32](#32) [#41](#41) [#49](#49) [#43](#43) [#56](#56) [#51](#51) [#57](#57) [#61](#61) [#67](#67) [#60](#60) [#70](#70) [#65](#65) [#71](#71) [#66](#66) [#58](#58) [#77](#77) [#81](#81)

### BREAKING CHANGES

* This feature will deprecate existing custom taxonomies, so any existing terms for those taxonomies will be lost.

To fix, we can convert terms from the deprecated taxonomies to standard post categories/tags via a migration script.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants