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.
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.
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.
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.
! isset( $attributes[ $name ] ) should be enough here, as it'll be false if ! isset( $attributes )
php/helpers.php
Outdated
|
|
||
| if ( ! isset( $block_lab_attributes[ $name ]['rows'] ) ) { | ||
| if ( ! $attributes || ! isset( $attributes[ $name ]['rows'] ) ) { |
There was a problem hiding this comment.
! isset( $attributes[ $name ]['rows'] ) should be enough, as it'll also be false if ! $attributes
|
@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.
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.