-
Notifications
You must be signed in to change notification settings - Fork 20
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
Implement ysacf_exclude_fields filter for legacy support #36
Implement ysacf_exclude_fields filter for legacy support #36
Conversation
I've tested this with the latest version of ACF Pro and it works! |
It's tested with 4 and 5! |
I like the general idea. Just some thoughts: For the the type blacklist we use an instance of Also the filter name is not in line with the rest of the filters. It should probably be So what about this:
Another thing I am wondering is if the naming might get confusing now. We have And then of course I think this PR should add tests for this feature:
And to close things up on a final note it seems as the last commits on the |
Thanks @kraftner ! Yes, I wondered about the filter name, but ideally we'd keep the old name as you suggest. It seems like a good idea to add this to the store and keep backwards compatibility that way. Yup, otherwise I pretty much agree with all of this! I'll take a gander at this tomorrow if I don't have time later today. One quick question: Should I use an instance of |
@kraftner I'm working on this now, the refactor works ok for manual tests but I'm having a spot of difficulty in implementing the unit tests for the feature. The method is essentially the same as the one used to test for the blacklist of types and I just can't seem to figure out what the issue is. I haven't worked with https://brain-wp.github.io/BrainMonkey/docs/wordpress-hooks-done.html before. Any advice on how to proceed?!
|
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.
Great, we're getting there. 😄
The order of the comments is a bit confusing here. If you open https://github.com/Yoast/yoast-acf-analysis/pull/36/files and read them as they appear in the source I think it is easier to understand.
If you have any further questions let me know.
$blacklist_name = new \Yoast_ACF_Analysis_String_Store(); | ||
|
||
$configuration = new \Yoast_ACF_Analysis_Configuration( | ||
$blacklist_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.
The first parameter passed to the constructor of Yoast_ACF_Analysis_Configuration
is the store for blacklist_type
and not blacklist_name
. As this is not what we're testing this should be
new \Yoast_ACF_Analysis_String_Store();
$this->field_selectors = $field_selectors; | ||
$this->blacklist_name = new Yoast_ACF_Analysis_String_Store(); |
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.
The Store for the blacklist should not be instantiated here but just like the other two stores in Yoast_ACF_Analysis
and passed as a parameter to the constructor.
Be aware - a lot of tests will fail after that change because they lack the third parameter sou you'll need to update those tests.
|
||
$configuration = new \Yoast_ACF_Analysis_Configuration( | ||
$blacklist_name, | ||
new \Yoast_ACF_Analysis_String_Store() |
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.
Now let's say you've changed the constructor of Yoast_ACF_Analysis_Configuration
to have the store for blacklist_name
as the 2nd parameter then you'd now put $blacklist_name
above that line.
|
||
$blacklist_name2 = new \Yoast_ACF_Analysis_String_Store(); | ||
|
||
$configuration->get_blacklist_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.
This line should be removed as we test this function in the assert at the end.
->once() | ||
->with( $blacklist_name ) | ||
->andReturn( $blacklist_name2 ); | ||
|
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.
So just to explain what this does:
It tests that:
- The filter with the name of
\Yoast_ACF_Analysis_Facade::get_filter_name( 'blacklist_name' )
... - ... is called once ...
- ... and gets the value of
$blacklist_name
passed ... - ... and makes the filter return
$blacklist_name2
Now you can also see why the test failed: As you created a new instance of \Yoast_ACF_Analysis_String_Store
in the constructor of \Yoast_ACF_Analysis_Configuration
but expected the filter to get $blacklist_name
passed (->with( $blacklist_name )
) the test failed.
// | ||
// $configuration = new \Yoast_ACF_Analysis_Configuration( | ||
// $store, | ||
// new \Yoast_ACF_Analysis_String_Store() |
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.
The same things I commented on testBlacklistNameFilter()
applies here.
@kraftner great! Thanks for such a detailed answer :D I've managed now to get my head around this now and all tests are currently passing. I'll get stuck into the nightcrawler tests next! |
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 great apart from that tiny mix-up!
inc/class-yoast-acf-analysis.php
Outdated
@@ -51,6 +51,7 @@ public function boot() { | |||
|
|||
$configuration = new Yoast_ACF_Analysis_Configuration( | |||
$this->get_blacklist_type(), | |||
$this->get_blacklist_type(), |
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 should be $this->get_blacklist_name()
.
When you start working on the Nightwatch tests you might run into #42. Just telling you so you don't waste your time. 😄 |
Hmm, at the moment all the Nightwatch tests are currently failing... I'm using your https://github.com/kraftner/yoast-acf-analysis-development for my local environment. All of the Nightwatch tests are failing with
|
Can you give me some more of that error output? This isn't really enough info to debug. |
[Tests / General / Basic] Test Suite✔ Element <#user_login> was visible after 94 milliseconds. Running: Text Field FAILED: 1 assertions failed and 1 passed (6.897s) [Tests / General / Content] Test Suite✔ Element <#user_login> was visible after 86 milliseconds. Running: WYSIWYG Field FAILED: 1 assertions failed and 1 passed (16.704s) |
Do you see the dummy fields added by the test when editing a Posts with your Browser? Does the plugin work there if you open it manually? There should be screenshots of failed tests in If still stuck please uncomment line 9 in |
Console showed that #38 was to blame for much of this. [SEVERE] 1502453153806 : http://yoast-acf-analysis.vvv.dev/wp-content/plugins/wordpress-seo/js/dist/select2/select2.full.min.js?ver=4.0.3 2:9236 "The select2('data') method was called on an element that is not using Select2." I applied the fix in #38 and got a bit further, but the tests still fail: ✖ Expected element <#snippet_title> text to contain: "4f12c824bde715b97da0651a8c27d184f167754f29363d5bd2099fee035f9874" - expected "contain '4f12c824bde715b97da0651a8c27d184f167754f29363d5bd2099fee035f9874'" but got: "%%cf_yoast_acf_analysis_text%%" FAILED: 1 assertions failed and 3 passed (10.368s) The Text field, WYSIWYG and URL tests all fail for the same reason. |
Ah, that explains why I didn't see that - I had ACF4 running. But we're almost there now. 😄 The error you are seeing is due to the changes to the tests introduced by #34 which rely on a feature of YoastSEO that isn't yet released. You'll need to install the trunk version of Yoast SEO. This got a bit messy when I did it yesterday, maybe @moorscode can shed some light on what the ideal workflow would be here. |
I guess it would've been better if the tests were conditional based on the availability of the functionality in Yoast SEO. The code introduced is checking if the functionality exists, and is not being used when an older version of Yoast SEO is present. |
Great work on the PR, one thing that I'm missing is a test which tests the |
I have just opened #46 which should probably get fixed to make work here easier. |
Hi @richardsweeney, I was wondering if you are currently working on this and if you could give an estimate on when you think you can complete this PR? |
Ping @atimmer for updates on this PR. |
Hi @moorscode - sorry been very busy with another project. I'll try very very hard to make time to work on this tomorrow. |
…list_name as for the blacklist_type
a83033a
to
2ebe382
Compare
Hey @moorscode ! I've added a unit test to test for the old filter which passes now. Unfortunately my unit tests are failing now - not one I've written:
How should I best proceed here do you think? |
I've tried installing the trunk version of WordPress SEO plugin, but no dice. https://github.com/Yoast/wordpress-seo/wiki/Using-Composer#setting-up-development-copy This version also makes all the Nightwatch tests fail :/ Hopefully without sounding like too much of a dork, I have to say this all makes contributing pretty difficult for a non Yoaster... All advice welcomed with open arms! |
Hmm, on second though my vagrant seems to have decided to remove all the field groups which probably explains why the Nightwatch tests keep failing. I'll try a clean provision to get them back. |
I think something went wrong when you merged in Also the trunk version should not be needed any more. This was resolved by the things mentioned in #36 (comment) |
Okay I think two things happened here:
|
Okay so I just created and merged #51 which should fix problem 2. |
@@ -53,7 +53,8 @@ public function boot() { | |||
} | |||
|
|||
$configuration = new Yoast_ACF_Analysis_Configuration( | |||
$this->get_blacklist(), | |||
$this->get_blacklist_type(), |
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.
Should be get_blacklist_name
Okay so I got your branch working:
|
As I already had the things mentioned in #36 (comment) done anyway I created a PR on your branch so you don't need to do it again. |
...and I have added a Nightwatch test for the name blacklist. If you can confirm that everything is working and merge stuff in your branch we can merge this PR and call it a day! 🎉 |
…fields-filter Helping out with Features/implement ysacf exclude fields filter
Hi @kraftner . Many thanks for the PR. I've merged in this now. All the unit tests pass, but my Nightwatch tests still don't work as my Vagrant box handily deleted the fields for me. I'll try to get this fixed, but ff you're happy and the tests pass for you then hopefully we can merge this and move forward with our lives :) |
Great! Concerning the Nightwatch tests: I'm a bit in a hurry right now, but can you try adding |
Patch for #35.