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

BUG: ep_weighting_configuration_for_search filter not applied if using fallback default weight config #3132

Closed
andyblackwell opened this issue Nov 14, 2022 · 4 comments
Assignees
Labels
bug Something isn't working reporter feedback
Milestone

Comments

@andyblackwell
Copy link

when using the ep_weighting_configuration_for_search filter, it is not applied when the plugin code needs to backfill with a default weight config for a post type
https://github.com/10up/ElasticPress/blob/4.3.1/includes/classes/Feature/Search/Weighting.php#L580-L590
https://github.com/10up/ElasticPress/blob/4.3.1/includes/classes/Feature/Search/Weighting.php#L531-L547

common case: when the weighting configs have not been manually saved in the admin
another possible use case: a new custom post type has been created since the last time the weighting config has been saved
in these cases the fallback to the default weight config will bypass the ep_weighting_configuration_for_search filter entirely, requiring a bit of a hack to also filter the defaults via ep_weighting_default_post_type_weights and having to be careful only to do that in the correct scenarios (only during search, and not while in the admin or while saving)

Steps to Reproduce

  1. have not saved in the weighting config admin
  2. use the ep_weighting_configuration_for_search filter, like as described in: https://elasticpress.zendesk.com/hc/en-us/articles/4402857301389
  3. the custom filter will not be applied, but rather the default weight config will be

Expected behavior
that the ep_weighting_configuration_for_search filter is applied regardless of whether or not the weighting config has been saved

Environment information

  • WordPress version: 6.0.2
  • ElasticPress version: 4.3.1
@felipeelia
Copy link
Member

Hi @andyblackwell,

I'm sorry but I'm having a hard time understanding the use case you have at hand here. The array manipulated by the ep_weighting_configuration_for_search filter has post types as indices, so the default config is only expected to be used when the filter did not foresee the presence of that post type.

common case: when the weighting configs have not been manually saved in the admin

The test introduced in #3303 checks that. I probably got your point wrongly but even when the option is empty (the weighting config was not saved), it will use the filter.

another possible use case: a new custom post type has been created since the last time the weighting config has been saved
in these cases the fallback to the default weight config will bypass the ep_weighting_configuration_for_search filter entirely, requiring a bit of a hack to also filter the defaults via ep_weighting_default_post_type_weights and having to be careful only to do that in the correct scenarios (only during search, and not while in the admin or while saving)

That is really not needed if you know the post type slug (and that should be the case, no?) Also, using the dashboard to change those values would always be preferable. Regarding "only during search, and not while in the admin or while saving", weighting is not applied during content saving (unless you are talking about something else?), and should not be used in admin unless the value passed through the ep_enable_do_weighting filter is changed.

Do you mind clarifying the scenario you have at hand a bit more? Thanks in advance!

@andyblackwell
Copy link
Author

andyblackwell commented Feb 9, 2023

added some comments to demonstrate the issue

/* in Weighting::do_weighting() */

$weight_config = $this->get_weighting_configuration();
// 1. if you haven't saved this config in the admin, $weight_config will be empty
// 2. if you've just deployed a new post type, and not yet re-saved this config 
//    in the admin , $weight_config[$post_type] will be empty

$weight_config = apply_filters( 'ep_weighting_configuration_for_search', $weight_config, $args );
// ^ we can filter and alter the values here, but if any post type configs are Not set then would 
// need to be prepared to set All values in the config, would be unable to just make tweaks to 
// the default config that ends up being used, which would be set below in a fallback

if ( ! isset( $weight_config[ $post_type ] ) ) {
	$weights = $this->get_post_type_default_settings( $post_type );
	// ^ seems like the fallback defaults should be pulled into $weight_config above,
	//  Before the `ep_weighting_configuration_for_search` filter is applied
} else {
	$weights = $weight_config[ $post_type ];
}
// example filter that just wants to tweak the current configs, 
// but won't apply when a post type doesn't have a saved config, 
// will just fallback to the default config without applying this filter
// for this example, we're just transferring the `post_content` value to `post_content_filtered`
add_filter('ep_weighting_configuration_for_search', function($weight_config) {
        foreach ($weight_config as &$post_type_config) {
            if (isset($post_type_config['post_content'])) {
                $post_type_config['post_content_filtered'] = $post_type_config['post_content'];
                unset($post_type_config['post_content']);
            }
        }
        return $weight_config;
});

To get around this, we had to add/remove another filter on the default config itself ep_weighting_default_post_type_weights, but had to be careful to only apply this one when the ep_weighting_configuration_for_search filter was about to be used, since we're just using the default post_content config from the admin and just transferring its value to post_content_filtered, we're careful altering the defaults to avoid removing the post_content config value while in the admin to configure and save that value

all that said, I personally don't need this fix anymore since the place I worked for did a big layoff this week 😢 ... and we had already worked around the issue, were just hoping to remove the hack/workaround in the future

@felipeelia
Copy link
Member

Hey @andyblackwell, I'm super sorry to read your last paragraph there and I hope you find a good place to work asap (10up is hiring, by the way.)

Going back to our problem, something like the code below should be enough. I'm still unclear about why you'd need to be careful with it, as it would only affect post types that didn't have any changes applied in the Weighting Dashboard.

add_filter(
	'ep_weighting_default_post_type_weights',
	function ( $defaults ) {
		$defaults['post_content_filtered'] = [
			'enabled' => true,
			'weight'  => 1,
		];
		return $defaults;
	}
);

Would it be possible to share the code you ended up with? Thanks

@github-actions
Copy link

This issue has been automatically closed because there has been no response to our request for more information. With only the information that is currently in the issue, we don't have enough information to take action. Please reach out if you have or find the answers we need so that we can investigate further. See this blog post on bug reports and the importance of repro steps for more information about the kind of information that may be helpful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working reporter feedback
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants