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

Implement ysacf_exclude_fields filter for legacy support #36

Conversation

richardsweeney
Copy link
Contributor

Patch for #35.

@viktorfroberg
Copy link
Contributor

I've tested this with the latest version of ACF Pro and it works!

@richardsweeney
Copy link
Contributor Author

It's tested with 4 and 5!

@viktorfroberg viktorfroberg added this to the 2.0 milestone Aug 9, 2017
@kraftner
Copy link
Contributor

kraftner commented Aug 9, 2017

I like the general idea. Just some thoughts:

For the the type blacklist we use an instance of Yoast_ACF_Analysis_String_Store and as this is pretty much the same as blacklist just with names instead of field types I think that for consistency reasons this should also use such a store.

Also the filter name is not in line with the rest of the filters. It should probably be Yoast_ACF_Analysis_Facade::get_filter_name( 'exclude_fields' ). Now I know that this would be a breaking change for anyone migrating from your plugin. Still I think we should have a clean filter that adheres to the naming scheme of the rest of the plugin.

So what about this:

  1. We call your legacy ysacf_exclude_fields filter with an empty array. Anything valid that comes from that is then added to the Yoast_ACF_Analysis_String_Store instance stored in $excluded_fields.
  2. After that we again filter this store with a filter name adhering to the naming convention of the plugin just the way we do in get_blacklist()
  3. Finally we should document and educate users to migrate to the new filter. Maybe even show some kind of warning if anything is hooked to ysacf_exclude_fields.

Another thing I am wondering is if the naming might get confusing now. We have blacklist for the type blacklist and exclude_fields for a field blacklist. I think we should make this consistent in naming, either calling them something like
blacklist_type and blacklist_name
or
exclude_type and exclude_name

And then of course I think this PR should add tests for this feature:

  • In ConfigurationTest for the PHP side of things
  • And new Nightwatch test that has a new field added to the dummy fields and tests that the content is not passed to Yoast SEO.

And to close things up on a final note it seems as the last commits on the features/acf5 branch didn't properly rebuild the JS so there is some noise in your PR . I've opened #37 for this. This should probably be fixed first and your PR rebased on a clean state of the branch.

@richardsweeney
Copy link
Contributor Author

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 Yoast_ACF_Analysis_String_Store for the store?

@kraftner
Copy link
Contributor

kraftner commented Aug 9, 2017

Great!

Yes I'd say use an instance of Yoast_ACF_Analysis_String_Store and have a look how its done for the blacklist here here and here.

Also if you find time I think fixing #37 first would be great so this PR becomes cleaner.

@richardsweeney
Copy link
Contributor Author

@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?!

Mockery\Exception\NoMatchingExpectationException: No matching handler found for Mockery_0::apply_filters_yoast_hyphen_acf_hyphen_analysis_slash_blacklist_name(object(Yoast_ACF_Analysis_String_Store)). Either the method was unexpected or its arguments matched no expected argument list for this method

https://github.com/richardsweeney/yoast-acf-analysis/blob/features/implement-ysacf_exclude_fields-filter/tests/php/unit/Configuration/ConfigurationTest.php#L96

Copy link
Contributor

@kraftner kraftner left a 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,
Copy link
Contributor

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();
Copy link
Contributor

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()
Copy link
Contributor

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();
Copy link
Contributor

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 );

Copy link
Contributor

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()
Copy link
Contributor

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.

@richardsweeney
Copy link
Contributor Author

richardsweeney commented Aug 11, 2017

@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!

Copy link
Contributor

@kraftner kraftner left a 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!

@@ -51,6 +51,7 @@ public function boot() {

$configuration = new Yoast_ACF_Analysis_Configuration(
$this->get_blacklist_type(),
$this->get_blacklist_type(),
Copy link
Contributor

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().

@kraftner
Copy link
Contributor

When you start working on the Nightwatch tests you might run into #42. Just telling you so you don't waste your time. 😄

@richardsweeney
Copy link
Contributor Author

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

Failed [ok]: (false == true) - expected "true" but got: "false"

@kraftner
Copy link
Contributor

Can you give me some more of that error output? This isn't really enough info to debug.

@richardsweeney
Copy link
Contributor Author

richardsweeney commented Aug 11, 2017

[Tests / General / Basic] Test Suite

✔ Element <#user_login> was visible after 94 milliseconds.
✔ Element <#user_pass> was visible after 37 milliseconds.
✔ Element <#wp-submit> was visible after 43 milliseconds.
✔ Element <#adminmenu #menu-dashboard .current> was visible after 41 milliseconds.

Running: Text Field
✔ Element <body.post-new-php> was visible after 124 milliseconds.
✖ Failed [ok]: (false == true) - expected "true" but got: "false"
at Object. (/Users/richardsweeney/Desktop/vagrant/www/yoast-acf-analysis-development/wp-content/plugins/yoast-acf-analysis/tests/js/system/helpers/logContains.js:19:9)

FAILED: 1 assertions failed and 1 passed (6.897s)

[Tests / General / Content] Test Suite

✔ Element <#user_login> was visible after 86 milliseconds.
✔ Element <#user_pass> was visible after 37 milliseconds.
✔ Element <#wp-submit> was visible after 33 milliseconds.
✔ Element <#adminmenu #menu-dashboard .current> was visible after 40 milliseconds.

Running: WYSIWYG Field
✔ Element <body.post-new-php> was visible after 138 milliseconds.
✖ Failed [ok]: (false == true) - expected "true" but got: "false"
at Object. (/Users/richardsweeney/Desktop/vagrant/www/yoast-acf-analysis-development/wp-content/plugins/yoast-acf-analysis/tests/js/system/helpers/logContains.js:19:9)

FAILED: 1 assertions failed and 1 passed (16.704s)

@kraftner
Copy link
Contributor

kraftner commented Aug 11, 2017

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 tests/js/system/screenshots. Maybe those hint at an issue.

If still stuck please uncomment line 9 in logContains.js, rerun the test and post the additional output.

@richardsweeney
Copy link
Contributor Author

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."
[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:9329 Uncaught TypeError: Cannot read property 'data' of undefined

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%%"
at module.exports (/Users/richardsweeney/Desktop/vagrant/www/yoast-acf-analysis-development/wp-content/plugins/yoast-acf-analysis/tests/js/system/helpers/replaceVars.js:11:20)
at Object.Text Field (/Users/richardsweeney/Desktop/vagrant/www/yoast-acf-analysis-development/wp-content/plugins/yoast-acf-analysis/tests/js/system/tests/general/basic.js:20:9)

FAILED: 1 assertions failed and 3 passed (10.368s)

The Text field, WYSIWYG and URL tests all fail for the same reason.

@kraftner
Copy link
Contributor

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.

@kraftner kraftner mentioned this pull request Aug 11, 2017
@moorscode
Copy link
Contributor

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.

@moorscode
Copy link
Contributor

Great work on the PR, one thing that I'm missing is a test which tests the ysacf_exclude_fields filter being used to add fields to the blacklist.

@kraftner
Copy link
Contributor

I have just opened #46 which should probably get fixed to make work here easier.

@kraftner
Copy link
Contributor

All blockers for this issue (#26, #42 and #46) are now resolved in features/acf-5, so you can now easier continue working on this.

Right now your PR has a conflict due to #31 so you'll probably need to rebase your work on top of features/acf-5.

@moorscode
Copy link
Contributor

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?

@moorscode
Copy link
Contributor

Ping @atimmer for updates on this PR.

@richardsweeney
Copy link
Contributor Author

Hi @moorscode - sorry been very busy with another project. I'll try very very hard to make time to work on this tomorrow.

@richardsweeney richardsweeney force-pushed the features/implement-ysacf_exclude_fields-filter branch from a83033a to 2ebe382 Compare August 17, 2017 14:16
@richardsweeney
Copy link
Contributor Author

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:

Yoast\AcfAnalysis\Tests\MainTest::testInvalidConfig
Error: Class 'AC_Yoast_SEO_ACF_Content_Analysis' not found

...yoast-acf-analysis-development/wp-content/plugins/yoast-acf-analysis/tests/php/unit/MainTest.php:21

How should I best proceed here do you think?

@richardsweeney
Copy link
Contributor Author

richardsweeney commented Aug 18, 2017

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!

@richardsweeney
Copy link
Contributor Author

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.

@kraftner
Copy link
Contributor

I think something went wrong when you merged in features/acf-5. Might be due to the fact that the (file)name of the main class changed. I'll have a look in a minute and report back asap.

Also the trunk version should not be needed any more. This was resolved by the things mentioned in #36 (comment)

@kraftner
Copy link
Contributor

Okay I think two things happened here:

  1. Can you try if a composer dump-autoload inside the plugin directory (!not the site root) fixes the failing unit test?
  2. There is an uncaught mistake in Use the constants used in the AC plugin #31 that fails the inclusion of the fields. I'm on a PR - arriving in a minute.

@kraftner
Copy link
Contributor

kraftner commented Aug 18, 2017

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(),
Copy link
Contributor

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

@kraftner
Copy link
Contributor

Okay so I got your branch working:

  1. Merge the features/acf-5 branch to have Fixes wrong path introduced in #31 #51.
  2. composer dump-autoload inside the plugin directory (!not the site root)
  3. You'll now get another failure in the unit test which can be fixed by doing Implement ysacf_exclude_fields filter for legacy support #36 (review)
  4. 🎉

@kraftner
Copy link
Contributor

kraftner commented Aug 18, 2017

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.

@kraftner
Copy link
Contributor

...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
@richardsweeney
Copy link
Contributor Author

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 :)

@kraftner
Copy link
Contributor

Great! Concerning the Nightwatch tests: I'm a bit in a hurry right now, but can you try adding AC_YOAST_ACF_ANALYSIS_ENVIRONMENT=development to the .env file. The name of the debug flag was also changed.

@atimmer atimmer merged commit b45831c into Yoast:features/acf-5 Aug 21, 2017
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.

6 participants