-
Notifications
You must be signed in to change notification settings - Fork 64
Replace global variables with data array in Plugin class #435
Conversation
Hi @lukecarbis, If it's alright, tomorrow is probably the earliest I could review this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question About Storing Data In Loader
Hi @lukecarbis,
This PR looks good, and it'll keep us from being stuck with these globals.
What do you think about storing the ->data
in the Loader
class? That should keep the data from being changed anywhere else, and it could still be accessed in helpers.php
.
Hope you had a great weekend, it was definitely productive 😄
@kienstra I've implemented your suggestion (great idea by the way), but I'm struggling with some failing integration tests. I'm kind of stuck – could you please take a look? |
Sure, @lukecarbis! The integration tests aren't very easy to maintain 😄 It might be a few days before I get to this, if that's alright. |
block_field() looks for the Loader in block_lab()->loader, but these tests were instantiating a new Loader. So remove the instantiation.
*/ | ||
global $block_lab_attributes, $block_lab_config; | ||
$attributes = block_lab()->loader->get_data( 'attributes' ); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This DocBlock was definitely helpful when there was a global, but do you think it's needed now that it's getting $attributes
with a normal method call?
And likewise for some other places in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 06d78c7
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks Good
A Few Small Points
Hi @lukecarbis,
Nice work! This looks good.
There are just a few points.
If it's alright, I pushed to this branch to get the Integration tests to pass. Sorry, I try not to do that without asking.
Would you mind if I pushed a PHPUnit test for Loader::get_data()
to this branch?
Thanks, Luke!
php/helpers.php
Outdated
|
||
if ( ! isset( $block_lab_attributes[ $name ] ) ) { | ||
if ( ! $attributes || ! isset( $attributes[ $name ] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
! isset( $attributes[ $name ] )
should be enough here, as it'll be false if ! isset( $attributes )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0fbf45a
php/helpers.php
Outdated
|
||
if ( ! isset( $block_lab_attributes[ $name ]['rows'] ) ) { | ||
if ( ! $attributes || ! isset( $attributes[ $name ]['rows'] ) ) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
! isset( $attributes[ $name ]['rows'] )
should be enough, as it'll also be false if ! $attributes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done in 0fbf45a
@kienstra please go ahead! |
Test that the data is as expected, including with filters.
Thanks, 3e14338 adds a test for |
@kienstra Ready for you to take another look. :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
Hi @lukecarbis,
This looks great! It's to merge when you'd like.
It'd be good to mention in the Changelog the new API for this, like
block_lab->loader->get_data( 'attributes' )
instead of $block_lab_attributes
Closes #425.