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

dev: Refactor attribute resolution into Data\BlockAttributeResolver #284

Closed

Conversation

justlevine
Copy link
Contributor

@justlevine justlevine commented Sep 17, 2024

What

This PR refactors attribute resolution handling out of \Blocks\Block.php and into a new \Data\BlockAttributeResolver class.

Additionally, method internals have been cleaned up and parameter/internal variables renamed for readability.

Why

Long term, this is part of fixing attribute resolution and performance / implementing a block Model. This work has been cherry-picked out for reviewability, as it contains no breaking changes or new features/fixes/enhancements.

How

Block::resolve_block_attributes_recursive() now calls Data\BlockAttributeResolver::resolve_block_attribute(). The method has been marked @internal since' we're probably going to change it once a Block model has been fully implemented.

@justlevine justlevine requested a review from a team as a code owner September 17, 2024 11:42
Copy link

changeset-bot bot commented Sep 17, 2024

⚠️ No Changeset found

Latest commit: 3a1157b

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@justlevine
Copy link
Contributor Author

Rebased post #279

Copy link
Collaborator

@jasonbahl jasonbahl left a comment

Choose a reason for hiding this comment

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

I pulled this down and the first query I tried to execute (just happened to have it loaded in my IDE already):

{
  post(id: 121, idType: DATABASE_ID) {
    id
    title
    date
    editorBlocks(flat: false) {
      __typename
      ...CoreTable
    }
  }
}

fragment CoreTable on CoreTable {
  anchor
  apiVersion
  attributes {
    textColor
    style
    lock
    head {
      cells {
        align
        content
        tag
        scope
      }
    }
    hasFixedLayout
    gradient
    foot {
      cells {
        align
        content
        scope
        tag
      }
    }
    fontSize
    fontFamily
    className
    caption
    borderColor
    body {
      cells {
        align
        content
        scope
        tag
      }
    }
    backgroundColor
    anchor
    align
  }
  renderedHtml
}

Led to this error:

    {
      "debugMessage": "WPGraphQL\\ContentBlocks\\Data\\BlockAttributeResolver::parse_query_source(): Argument #3 ($attribute_values) must be of type array, null given, called in /Users/jason.bahl/Sites/libs/wp-graphql-content-blocks/includes/Data/BlockAttributeResolver.php on line 48",
      "message": "Internal server error",
      "extensions": {
        "category": "internal"
      },
      "locations": [
        {
          "line": 20,
          "column": 5
        }
      ],
      "path": [
        "post",
        "editorBlocks",
        2,
        "attributes",
        "head"
      ],
      "trace": [
        {
          "file": "/Users/jason.bahl/Sites/libs/wp-graphql-content-blocks/includes/Data/BlockAttributeResolver.php",
          "line": 48,
          "call": "WPGraphQL\\ContentBlocks\\Data\\BlockAttributeResolver::parse_query_source('<figure class=\"wp-block-table\"><table><thead><tr><th class=\"has-text-align-center\" data-align=\"center\">Header Label 1</th><th></th></tr></thead><tbody><tr><td class=\"has-text-align-center\" data-align=\"center\">1:1</td><td>1:2</td></tr><tr><td class=\"has-text-align-center\" data-align=\"center\">2:1</td><td>2:2</td></tr></tbody><tfoot><tr><td class=\"has-text-align-center\" data-align=\"center\">Footer Label 1</td><td>Footer Label 2</td></tr></tfoot></table></figure>', array(5), null)"
        },

@justlevine
Copy link
Contributor Author

@jasonbahl thanks for the catch! Fixed in 7b7599f

I also rebased the plugin on #290 (so CoreVideoTest actually runs now), which allowed me to find/fix the regression in #229 that was causing attributes to return null/incorrect.

@justlevine
Copy link
Contributor Author

justlevine commented Sep 23, 2024

@jasonbahl thanks for the catch! Fixed in 7b7599f

I also rebased the plugin on #290 (so CoreVideoTest actually runs now), which allowed me to find/fix the regression in #229 that was causing attributes to return null/incorrect.

#286 got merged directly, so the remaining relevant changes here are just the above (diff)

Reminder: #290 still needs to be merged first

@justlevine
Copy link
Contributor Author

This was merged as part of #293

@justlevine justlevine deleted the dev/refactor-BlockAttributeResolver branch September 26, 2024 20:32
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.

CoreVideo attributes controls always return true
2 participants