Files: 
CommentFileSizeAuthor
#92 interdiff-90-92.diff4.88 KBianthomas_uk
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-90-92.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]
#92 1831632-92-convert-node_rank_R-to-cmi.patch8.51 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 59,393 pass(es).
[ View ]
#90 1831632-90-convert-node_rank_R-to-cmi.patch6.89 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 59,780 pass(es).
[ View ]
#90 interdiff-1831632-81-90.txt5.05 KBianthomas_uk
#81 1831632-81-convert-node_rank_R-to-cmi.patch8.51 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 59,369 pass(es).
[ View ]
#72 1831632-72-convert-node_rank_R-to-cmi.patch17.42 KBianthomas_uk
PASSED: [[SimpleTest]]: [MySQL] 58,429 pass(es).
[ View ]
#72 1831632-interdiff-69-72.txt7.14 KBianthomas_uk
#69 66-69-interdiff.txt9.38 KBalexpott
#69 1831632-69-convert-node_rank_R-to-cmi.patch17.37 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 58,848 pass(es).
[ View ]
#66 1831632-66-convert-node_rank_R-to-cmi.patch13.4 KBalexpott
FAILED: [[SimpleTest]]: [MySQL] 59,139 pass(es), 2 fail(s), and 0 exception(s).
[ View ]
#66 65-66-interdiff.txt1.36 KBalexpott
#65 1831632-65-convert-node_rank_R-to-cmi.patch13.07 KBalexpott
PASSED: [[SimpleTest]]: [MySQL] 59,265 pass(es).
[ View ]
#63 1831632-62-convert-node_rank_R-to-cmi.patch10.64 KBznerol
PASSED: [[SimpleTest]]: [MySQL] 58,884 pass(es).
[ View ]
#61 1831632-60-convert-node_rank_R-to-cmi.patch10.64 KBznerol
FAILED: [[SimpleTest]]: [MySQL] 58,175 pass(es), 180 fail(s), and 21 exception(s).
[ View ]
#61 interdiff.txt14.53 KBznerol
#55 1831632-54-convert-node_rank_R-to-cmi.patch10.57 KBznerol
PASSED: [[SimpleTest]]: [MySQL] 57,592 pass(es).
[ View ]
#53 1831632-52-convert-node_rank_R-to-cmi.patch10.57 KBznerol
FAILED: [[SimpleTest]]: [MySQL] 57,577 pass(es), 1 fail(s), and 0 exception(s).
[ View ]
#42 1831632-42-convert-node_rank_R-to-cmi.patch10.49 KBJon Pugh
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831632-42-convert-node_rank_R-to-cmi.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#39 1831632-39-convert-node_rank_R-to-cmi.patch10.4 KBJon Pugh
PASSED: [[SimpleTest]]: [MySQL] 57,529 pass(es).
[ View ]
#38 1831632-38-convert-node_rank_R-to-cmi.patch10.46 KBJon Pugh
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#37 1831632-36-convert-node_rank_R-to-cmi.patch0 bytesJon Pugh
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
#33 1831632-33-convert-node_rank_R-to-cmi.patch8.52 KBACF
PASSED: [[SimpleTest]]: [MySQL] 55,899 pass(es).
[ View ]
#30 1831632-28-convert-node_rank_R-to-cmi.patch8.5 KBznerol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831632-28-convert-node_rank_R-to-cmi.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#29 1831632-27-convert-node_rank_R-to-cmi.patch8.32 KBznerol
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]
#23 1831632-22-convert-node_rank_R-to-cmi.patch8.29 KBznerol
PASSED: [[SimpleTest]]: [MySQL] 50,584 pass(es).
[ View ]
#20 1831632-19-convert-node_rank_R-to-cmi.patch7.87 KBznerol
PASSED: [[SimpleTest]]: [MySQL] 50,645 pass(es).
[ View ]
#16 1831632-15-convert-node_rank_R-to-cmi.patch9.2 KBznerol
PASSED: [[SimpleTest]]: [MySQL] 50,809 pass(es).
[ View ]
#13 1831632-12-convert-node_rank_R-to-cmi.patch9.17 KBznerol
FAILED: [[SimpleTest]]: [MySQL] 50,279 pass(es), 19 fail(s), and 19 exception(s).
[ View ]
#10 1831632-9-convert-node_rank_R-to-cmi.patch9.17 KBznerol
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/statistics/statistics.install.
[ View ]
#5 1831632-4-convert-node_rank_R-to-cmi.patch10.7 KBznerol
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831632-4-convert-node_rank_R-to-cmi.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]
#1 1831632-convert-node_rank_R-to-cmi.patch9.11 KBznerol
FAILED: [[SimpleTest]]: [MySQL] 47,590 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

Comments

znerol’s picture

Status:Active» Needs review
StatusFileSize
new9.11 KB
FAILED: [[SimpleTest]]: [MySQL] 47,590 pass(es), 12 fail(s), and 0 exception(s).
[ View ]

Patch attached. Note that in order to make the UI work, #1831802: Allow implementors of hook_search_admin to add #submit on the form is necessary.

Status:Needs review» Needs work

The last submitted patch, 1831632-convert-node_rank_R-to-cmi.patch, failed testing.

znerol’s picture

Tests actually fail as a result of #1831802: Allow implementors of hook_search_admin to add #submit on the form. The reason is that node_search_admin attaches a $form['#submit'] handler which overrides the submit handler of the main form. Tests succeed when the other patch gets commited.

znerol’s picture

Assigned:Unassigned» znerol

Writing upgrade-test as illustrated in http://drupal.org/node/1667896

znerol’s picture

StatusFileSize
new10.7 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831632-4-convert-node_rank_R-to-cmi.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Implement upgrade tests, same restriction on test-results holds like in #3.

znerol’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:-Configuration system

The last submitted patch, 1831632-4-convert-node_rank_R-to-cmi.patch, failed testing.

Berdir’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, 1831632-4-convert-node_rank_R-to-cmi.patch, failed testing.

znerol’s picture

StatusFileSize
new9.17 KB
FAILED: [[SimpleTest]]: [MySQL] Invalid PHP syntax in core/modules/statistics/statistics.install.
[ View ]

Reroll.

znerol’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1831632-9-convert-node_rank_R-to-cmi.patch, failed testing.

znerol’s picture

StatusFileSize
new9.17 KB
FAILED: [[SimpleTest]]: [MySQL] 50,279 pass(es), 19 fail(s), and 19 exception(s).
[ View ]

Another reroll.

znerol’s picture

Status:Needs work» Needs review

Status:Needs review» Needs work

The last submitted patch, 1831632-12-convert-node_rank_R-to-cmi.patch, failed testing.

znerol’s picture

StatusFileSize
new9.2 KB
PASSED: [[SimpleTest]]: [MySQL] 50,809 pass(es).
[ View ]

Hm. Probably update_variables_to_config does not like it if second parameter ($variable_map) is empty?

znerol’s picture

Status:Needs work» Needs review
Berdir’s picture

Status:Needs review» Needs work
+++ b/core/modules/comment/comment.installundefined
@@ -394,6 +394,15 @@ function comment_update_8004() {
+  update_variables_to_config('node.settings', array(
+    'node_rank_comments' => 'search_rank.comments',
+  ));

I think the config setting should go in a separate file provided by comment.module. We decided a while ago that modules should not extend config files from other modules.

Especially because comment is currently refactored to support all entities.

+++ b/core/modules/node/node.installundefined
@@ -707,6 +701,32 @@ function node_update_8013() {
+ * Moves variables node_rank_relevance, node_rank_sticky, node_rank_promote and
+ * node_rank_recent to config node.settings.search_rank.

I think this can be shortened to "node search rank variables" or something like that.

+++ b/core/modules/node/node.installundefined
@@ -707,6 +701,32 @@ function node_update_8013() {
+  $rank_variables = db_select('variable', 'v')
+    ->fields('v', array('name'))
+    ->condition('v.name', db_like($oldprefix) . '%', 'LIKE')
+    ->execute()
+    ->fetchCol();
+
+  $config = array();
+  foreach ($rank_variables as $rank_variable) {
+    $config[$rank_variable] = substr_replace($rank_variable, $newprefix, 0, $oldpfxlen);

Hm, haven't checked the older patches, is this necessary?

I thought that update_variables_to_config() is able to handle non-existing variables and defaults...

+++ b/core/modules/statistics/statistics.installundefined
@@ -173,3 +173,12 @@ function statistics_update_8002() {
+ */
+function statistics_update_8003() {
+  update_variables_to_config('node.settings', array(
+    'node_rank_views' => 'search_rank.views',

Same for this one then as for comments.

+++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.phpundefined
@@ -99,6 +99,11 @@ public function testVariableUpgrade() {
+    $expected_config['node.settings'] = array(
+      'search_rank.promote' => 10,
+      'search_rank.custom_from_contrib' => 5,

Ah I see, that's why you did it dynamically.

That might actually be irrelevant given that those settings should not be in node.settings... I'd instead of custom_from_contrib, let's add tests for comment and statistics instead...

znerol’s picture

Thanks for the review.

I search_rank.x is actually really a setting for the node module. There is no such thing for e.g. users (the only other entity which is searchable in core). So instead of moving search_rank.comments and search_rank.views over into comment.settings and statistics.settings respectively we are better off by merging the update functions into node_update_X.

The same reasoning could be applied to the dynamic update function. After all we deal with ranking presets for node-search. Contrib modules never need to touch this variables in drupal 7. However that will have to change for 8 anyway, so it is absolutely ok to leave responsibility for updating non-core ranking score settings to contrib.

znerol’s picture

StatusFileSize
new7.87 KB
PASSED: [[SimpleTest]]: [MySQL] 50,645 pass(es).
[ View ]

The patch actually looks a bit saner, because we do not touch comment.install and statistics.install anymore.

znerol’s picture

Status:Needs work» Needs review
sun’s picture

+++ b/core/modules/node/config/node.settings.yml
@@ -1 +1,6 @@
+search_rank:
+    promote: '0'
+    recent: '0'

Hm.

1) Would it make sense to move these settings into a new node.search config file? They are only needed when Search module is enabled. Otherwise, they're ignored/unused. E.g., how about this:

node.search.yml:
rank:
  promote: '0'
  recent: ...

2) The indentation for sub-keys should be 2 spaces instead of 4. (as usual)

+++ b/core/modules/node/node.install
@@ -707,6 +701,21 @@ function node_update_8013() {
+    'node_rank_comments' => 'search_rank.comments',
+    'node_rank_views' => 'search_rank.views',

1) node_rank_comments belongs to Comment module, not Node module. Therefore, it should be in a comment.search config file, with separate upgrade path.

2) node_rank_views belongs to Statistics module, if I'm not mistaken. It should be in a separate statistics.search config file with separate upgrade path.

znerol’s picture

StatusFileSize
new8.29 KB
PASSED: [[SimpleTest]]: [MySQL] 50,584 pass(es).
[ View ]

Ok, put node search ranking into a separate yml.

Concerning comments and statistics. I understand the confusion. I tried to clarify the reasoning why this settings belong into node-scope in #19. When rank.comments is moved into comments.search.yml two problems arise: First we lose the reference to the node module. The setting at hand is about whether nodes should be promoted in search results depending on the number of comments. It is not about searching comments or searching in comments at all. Second the configuration form for node-ranking as well as the ranking-code itself might become much more complex when we scatter those settings across multiple modules. The functions _node_rankings, node_search_admin and node_search_admin_submit will need methods to collect rank settings from multiple places.

The ranking mechanism only exists for nodes. And the settings controlling the way search results are scored therefore are node-settings. The number of comments or views is actually a property of a node, even if those values depend on data from other modules.

Berdir’s picture

Right, the settings are about nodes. But they are defined by comment and statistics module. And it was defined a while ago that yml files should only contain stuff from the module it lives in. So that they can provide meta information about it, for example.

The comment settings for node types will for example also live in a separate file.

znerol’s picture

The problem here is that unlike in the case of comment settings for node types, the comment module does not touch the comment node-ranking setting. The only involvement the comment module has with node-ranking is its implementation of hook_ranking where it provides the information the node module needs to execute the ranking. The ranking parameter (the comment node-ranking setting) is configured via the node module and should remain there.

If we move rank.comments to comments.search.yml, we still never access that from the comments module. We only complicate accessing it from node module.

sun’s picture

@znerol: Thank you for clarifying the problem space!

By now, it's pretty clear that something is completely off with those node search ranking settings. ;)

Perhaps the most pragmatic way to unblock this innocent issue would be to keep those settings in node.search, but additionally move/merge the comment_ranking() and statistics_ranking() back into node_ranking(), both wrapped in a corresponding module_exists().

That would at least bring some clarity into this confusing hook_ranking() implementation space for now. I can only guess that we should/would want to actually migrate that search ranking stuff from Node module to Entity module for D8 anyway (but I'm not aware of any issue).

What do you think?

znerol’s picture

I don't think that merging the hook_ranking implementations will do any good. The hook implementations extend the node-search algorithm with more scoring options. Whether or not these options are used is up to node-search configuration. Even if we'd extract node_search_execute into entity-space, the mechanism would be the same.

znerol’s picture

Ok, another attempt to find a solution for the naming-/domain issues. In order to clarify my points I'll try to relate the current search implementation with the domain model from Search API. The three principal concepts there are Server, Index and Entity. Cardinality from Index to Entity is 1:1, while that one from Index to Server is n:1. Lets simplify and say that the Index serves as a bridge between Entity and Server. One responsibility of the Index concept is to hold per Entity(-type) settings related to search (like boost values), such that neither the Server needs to know anything about Entity nor vice versa.

Regrettably the separation in core is not as clean as over at Search API. In the core search implementation there is only one search Server (in Search API-speak) and exactly one Index per supported entity type (node, user). Also there is no Index-type thing mediating between the Entity and the Server. The first problem we have now is that it is hard to decide on which side we want to drop off the node-search-ranking (aka boost) settings (see #18...#25). I feel that search.node.yml would actually be more appropriate than node.search.yml.

The second problem is that the ranking-mechanism for node-search is extendable in a funny way. We all enjoy the benefits of hook_X_alter in many situations. The case there is easy: The module implementing the hook is also responsible for providing an UI and storage for settings governing the alter-behavior. However hook_ranking does not work like that. It is more like hook_X_info: it provides details on how the modules data can be used by a third party. There is neither logic nor an UI implemented in comment and statistics modules influencing the way nodes are ranked in seach results. The only thing those modules provide is the information necessary for the node-search implementation such that it can use comment and statistics data. That said, I think it is correct to leave hook_ranking and its implementations where they are (disregarding the proposal in #26).

znerol’s picture

StatusFileSize
new8.32 KB
FAILED: [[SimpleTest]]: [MySQL] Setup environment: Test cancelled by admin prior to completion.
[ View ]

Reroll of #22 in order to keep up with head. Changes proposed in #28 will follow in a separate patch.

znerol’s picture

StatusFileSize
new8.5 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831632-28-convert-node_rank_R-to-cmi.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

Patch with changes here. Comments welcome.

ACF’s picture

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, 1831632-28-convert-node_rank_R-to-cmi.patch, failed testing.

ACF’s picture

Status:Needs work» Needs review
StatusFileSize
new8.52 KB
PASSED: [[SimpleTest]]: [MySQL] 55,899 pass(es).
[ View ]

Just a re-roll.

jibran’s picture

Jon Pugh’s picture

Assigned:znerol» Jon Pugh
Status:Needs review» Needs work

I am currently re-rolling along with #1938920: Convert node_search_admin theme tables to table #type, they collide a bit.

Jon Pugh’s picture

Status:Needs work» Needs review

This patch includes work done at #1938920: Convert node_search_admin theme tables to table #type, rerolled over current head.

Jon Pugh’s picture

StatusFileSize
new0 bytes
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]
Jon Pugh’s picture

StatusFileSize
new10.46 KB
FAILED: [[SimpleTest]]: [MySQL] Repository checkout: failed to checkout from [git://git.drupal.org/project/drupal.git].
[ View ]

Oops! Battery failed in the middle of my git diff'ing

Jon Pugh’s picture

StatusFileSize
new10.4 KB
PASSED: [[SimpleTest]]: [MySQL] 57,529 pass(es).
[ View ]

Re-roll with newlines at end of files.

Jon Pugh’s picture

Status:Needs review» Needs work

Crap it still doesn't seem to save the configs for the CONTENT RANKING settings...

gonna need some help on this one.

alexpott’s picture

Status:Needs work» Needs review
Jon Pugh’s picture

StatusFileSize
new10.49 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch 1831632-42-convert-node_rank_R-to-cmi.patch. Unable to apply patch. See the log in the details link for more information.
[ View ]

It was saving a bad array. This fixes it!

Jon Pugh’s picture

BTW....

Is it ok to append to an array like so? (according to our coding standards?) wasn't sure...

<?php
  $ranks
= array();
  foreach (
$form_state['values']['content_ranking']['factors'] as $factor){
   
$ranks += $factor;
  }
?>
dawehner’s picture

While changing all this lines we could replace it with Drupal::config()

Jon Pugh’s picture

It does use config(), that was just a snippet. The problem is that the form submission generates an array that doesn't match the structure of the config, so I used the foreach to simplify the array so it matches the config.

+ /**
+ * Submit callback for search admin form callback.
+ */
+function node_search_admin_submit($form, &$form_state) {
+ $ranks = array();
+ foreach ($form_state['values']['content_ranking']['factors'] as $factor){
+ $ranks += $factor;
+ }
+ config('search.node')
+ ->set('rank', $ranks)
+ ->save();
+}

dawehner’s picture

Also just in general, is there a reason why we not prefix it with node instaed? So node.search instead of search.node. All this code is in node.module, which deals with that.

znerol’s picture

@dawehner see #28.

jibran’s picture

Status:Needs review» Needs work
Issue tags:+Configuration system

The last submitted patch, 1831632-42-convert-node_rank_R-to-cmi.patch, failed testing.

dawehner’s picture

Issue tags:+Needs reroll

.

crazybutable’s picture

Jon Pugh: This issue has needed a re-roll for a week, are you still working on it, or can somebody else do the re-roll? :)

znerol’s picture

Assigned:Jon Pugh» znerol

Working on a reroll.

znerol’s picture

Status:Needs work» Needs review
StatusFileSize
new10.57 KB
FAILED: [[SimpleTest]]: [MySQL] 57,577 pass(es), 1 fail(s), and 0 exception(s).
[ View ]

Status:Needs review» Needs work

The last submitted patch, 1831632-52-convert-node_rank_R-to-cmi.patch, failed testing.

znerol’s picture

Status:Needs work» Needs review
StatusFileSize
new10.57 KB
PASSED: [[SimpleTest]]: [MySQL] 57,592 pass(es).
[ View ]

Fixed conversion of config to Drupal::config in test case.

znerol’s picture

Assigned:znerol» Unassigned
mtift’s picture

@@ -115,7 +115,7 @@ function hook_search_admin() {
-  $ranks = config('node.settings')->get('search_rank');
+  $ranks = Drupal::config('search.node')->get('rank');

This was changed to:

$ranks = Drupal::config('node.settings')->get('search_rank');

So this patch needs a re-roll.

znerol’s picture

@mtift: Thanks for the review. Actually node.settings is wrong and search.node is correct. This is a necessary documentation update which looks like it was introduced accidentally.

Berdir’s picture

Probably makes sense to wait with a re-roll until #2003482: Convert hook_search_info to plugin system

jhodgdon’s picture

Status:Needs review» Needs work

That other issue is in... I'm not sure how much of this still needs to be done, but it's time to reroll this one.

znerol’s picture

Status:Needs work» Needs review
StatusFileSize
new14.53 KB
new10.64 KB
FAILED: [[SimpleTest]]: [MySQL] 58,175 pass(es), 180 fail(s), and 21 exception(s).
[ View ]

This is a reroll against head. Obviously now that node ranking went into NodeSearch plugin, the configuration needs to be in node.search and not in search.node.

Status:Needs review» Needs work

The last submitted patch, 1831632-60-convert-node_rank_R-to-cmi.patch, failed testing.

znerol’s picture

Status:Needs work» Needs review
StatusFileSize
new10.64 KB
PASSED: [[SimpleTest]]: [MySQL] 58,884 pass(es).
[ View ]

Fix namespace on calls to Drupal::config.

catch’s picture

Priority:Normal» Critical
alexpott’s picture

Issue tags:-Needs reroll
StatusFileSize
new13.07 KB
PASSED: [[SimpleTest]]: [MySQL] 59,265 pass(es).
[ View ]

I think we should take a slightly different approach here and use the plugin configuration system here. I'm not going with full config entities as that seems overload but patch attached will read in a search.plugin.plugin_id.yml for any search plugin and add what's contained within to the plugin's configuration array.

alexpott’s picture

StatusFileSize
new1.36 KB
new13.4 KB
FAILED: [[SimpleTest]]: [MySQL] 59,139 pass(es), 2 fail(s), and 0 exception(s).
[ View ]

Doh!

pwolanin’s picture

Status:Needs review» Needs work

ok, so I see where you are going with this, but I think it could be improved:

I think this change is wrong:

-      $container->get('config.factory')->get('search.settings'),
+      $container->get('config.factory'),

I'd either leave it as-is, or better yet maybe merge it into the configuration array used to instantiate the plugin so we totally get this out of the dependency list.

Minor/personal taste:

+    // Ensure we return an array. If the plugin has no configuration this will
+    // be null.
+    if (!is_array($configuration)) {
+      $configuration = array();
+    }

I'd personally use if (empty($configuration))

pwolanin’s picture

I can do a quick re-roll with those changes

alexpott’s picture

Status:Needs work» Needs review
StatusFileSize
new17.37 KB
PASSED: [[SimpleTest]]: [MySQL] 58,848 pass(es).
[ View ]
new9.38 KB

This will need to be rerolled after #2086201: Use PluginFormInterface instead of one-off form methods for search plugins gets in.

Ok after talking to @pwolanin lets see if we can avoid injecting the config factory. And I just noticed the SearchExtraTypeSearch which can now also use the new search plugin configuration system.

pwolanin’s picture

Yes, I like this better as long as we get a config object.

Minor, but I might use $this->getPluginId() since it's guaranteed by the interface while the protected variable name is not?

jhodgdon’s picture

Status:Needs review» Needs work

I read through the code and docs, and it all looks pretty clear to me. One minor thing to fix:

+   * @param string $plugin_id
+   *   The id of the plugin to get configuration for.

This is in SearchPluginManager.php -- please use "ID" not "id" (pet peeve of mine: id is like ego, superego, id; ID means identifier... it's all over core and it's just wrong and I hate it!).

Actually, come to think of it:

+  /**
+   * Gets plugin configuration stored in the configuration management system.
+   *
+   * @param string $plugin_id
+   *   The id of the plugin to get configuration for.
+   *
+   * @return array
+   *   An array of plugin configuration.
+   */
+  protected function getConfiguration($plugin_id) {
+    $configuration = array();
+    $configuration['search.plugin.' . $plugin_id] = $this->configFactory->get('search.plugin.' . $plugin_id);
+    $configuration['search.settings'] = $this->configFactory->get('search.settings');
+    return $configuration;
+  }

Could we add something to the @return to explain that you get the search plugin and generic search settings in the return value? And the one-line description just says it is getting plugin configuration, which doesn't seem accurate.

ianthomas_uk’s picture

StatusFileSize
new7.14 KB
new17.42 KB
PASSED: [[SimpleTest]]: [MySQL] 58,429 pass(es).
[ View ]

Here's a reroll using not much more intelligence than git itself. The interdiff contains the changes I made to resolve the conflicts. Leaving at needs work until I've given it more of a test myself.

Oh, I did manage to slip in the id > ID change though.

ianthomas_uk’s picture

Continuing from #71 re getConfiguration(). This function exists to provide parameters for createInstance(), but it's not a pattern that I can find anywhere else in Drupal and I think this might have been an unintentional change in #69.

FactoryInterface defines the $configuration parameter of createInstance as "An array of configuration relevant to the plugin instance.", so I think this is intended to be a potentially multi-level array, rather than an array of config objects. I think we would do this just by adding ->get() to these calls (the equivilent code before #69 did call ->get()).

+    $configuration['search.plugin.' . $plugin_id] = $this->configFactory->get('search.plugin.' . $plugin_id)<strong>->get()</strong>;
+    $configuration['search.settings'] = $this->configFactory->get('search.settings')<strong>->get()</strong>;

I'm not sure what impact this would have on the created instance though.

jibran’s picture

Status:Needs work» Needs review

test

pwolanin’s picture

Might need some work.

+    $config = $this->configuration['search.plugin.' . $this->pluginId];

Seems like we'd want a namespace for "ranking" rather than assuming they will always be the only top-level config for node search?

alexpott’s picture

@pwolain are you sure about #75

The original variables were very node specific - node_rank_promote, node_rank_views, node_rank_comments, node_rank_custom. I'm not sure that we should be generalising them here.

ianthomas_uk’s picture

  1. +++ b/core/modules/node/lib/Drupal/node/Plugin/Search/NodeSearch.php
    @@ -272,14 +260,14 @@ protected function addNodeRankings(SelectExtender $query) {
           foreach ($ranking as $rank => $values) {
    -        // @todo - move rank out of drupal variables.
    -        if ($node_rank = variable_get('node_rank_' . $rank, 0)) {
    +        $rank = $this->configuration['search.plugin.' . $this->pluginId]->get('rank.' . $rank);
    +        if (!empty($rank)) {
               // If the table defined in the ranking isn't already joined, then add it.
               if (isset($values['join']) && !isset($tables[$values['join']['alias']])) {
                 $query->addJoin($values['join']['type'], $values['join']['table'], $values['join']['alias'], $values['join']['on']);
               }
               $arguments = isset($values['arguments']) ? $values['arguments'] : array();
    -          $query->addScore($values['score'], $arguments, $node_rank);
    +          $query->addScore($values['score'], $arguments, $rank);
             }
           }

    $rank is used both for the index of the array being looped over and for the value returned from config. I'd suggest renaming the latter back to $node_rank.

  2. +++ b/core/modules/system/lib/Drupal/system/Tests/Upgrade/SystemUpgradePathTest.php
    @@ -168,6 +168,13 @@ public function testVariableUpgrade() {
    +    $expected_config['search.plugin.node_search'] = array(
    +      'rank.promote' => '10',
    +      'rank.views' => '5',
    +      'rank.comments' => '-2',
    +      'rank.custom' => '6',
    +    );
    +

    These should be integers now, see #1653026: [META] Use properly typed values in module configuration

It's a shame the change suggested in #67 to remove the config factory dependency wasn't split into it's own task, as it's made the patch more complicated than it needed to be, but it's probably not worth splitting them now.

Note that there are unaddressed review comments from #70 onwards.

ianthomas_uk’s picture

Status:Needs review» Needs work

Defaults should be provided by .yml files, not using ternary operators.

I don't think we should be dynamically adding the plugin ID in this line either:
$this->configFactory->get('search.plugin.' . $plugin_id);
I think we should be asking the plugin for it's config entity (although I'm still a bit new to this stuff).

ianthomas_uk’s picture

+++ b/core/modules/search/lib/Drupal/search/SearchPluginManager.php
@@ -117,4 +117,29 @@ public function pluginAccess($plugin_id, AccountInterface $account) {
+  protected function getConfiguration($plugin_id) {
+    $configuration = array();
+    $configuration['search.plugin.' . $plugin_id] = $this->configFactory->get('search.plugin.' . $plugin_id);
+    $configuration['search.settings'] = $this->configFactory->get('search.settings');
+    return $configuration;
+  }

While I can see the point of this, it doesn't seem to be how getConfiguration is meant to be used. Looking at other plugins, I think getConfiguration should return just the configuration for the plugin as an array (i.e. the contents of $this->configFactory->get('search.plugin.' . $plugin_id)).

I don't think the plugins should be accessing search.settings directly at all, rather the search module should make the necessary settings available.

I'll keep looking at this tomorrow, in particular I couldn't see how the configuration from other plugins was actually saved to/read from disk.

ianthomas_uk’s picture

I've split the problem that @alexpott and @pwolanin were working on in #69 into #2123073: Move index.cron_limit setting to NodeSearch and would really appreciate people's thoughts on that. I'm happy to write patches for both issues once there is consensus on that issue (which, if accepted, would simplify the patch for this issue).

ianthomas_uk’s picture

Status:Needs work» Needs review
StatusFileSize
new8.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,369 pass(es).
[ View ]

I've got a much better understanding of what's going on here now and I don't agree with the approach taken from #65 onwards. It seems to me that you've got two basic approaches you can take:
a) Pass everything in as parameters on the constructor, store them as properties and go back later to see if they've been changed and need saving to disk (aka the config entity approach).
b) Handle everything internally using \Drupal::config()

This seems an odd hybrid of the two, in that it looks like you're passing in configuration, but really you're passing in a reference to the configuration, and that reference is always going to point to the same place! (NodeSearch will always be search.plugin.node_search)

The patch was also made more complicated by some semi-related refactoring and an odd variable name change. I've already split off the search.settings removal into it's own issue. I don't see the need for SearchExtraTypeSearch->$configuration, but it's not causing any problems so I'll leave that for now.

Once all that was undone we're left with what was actually quite a simple conversion. I've not done an interdiff, as it would be bigger than the diff. This is coded blind at this stage, but I've run out of time today. I'll hopefully get a chance to test it myself tomorrow. Setting to needs review for the test bots.

znerol’s picture

@ianthomas_uk: Is there a conceptual diff between #63 and #81?

ianthomas_uk’s picture

Conceptually they are very similar, although the config name has changed from node.search to search.plugin.node_search. I created #81 by removing changes from #72.

Other differences that I need to investigate:
- No longer converting node_rank_comments or node_rank_views, I assume these variables are no longer used.
- #63 has some changes that don't seem relevant to this issue, e.g. removal of theme_node_search_admin(). I'll check if any of those are still wanted.
- #63 doesn't always deal with all the variables, e.g. drupal-7.system.database.php only lists 3 of 6, #81 lists 4 of 4.

znerol’s picture

No longer converting node_rank_comments or node_rank_views, I assume these variables are no longer used.

I still see hook_ranking implementations in comment.module and statistics.module, therefore I'm pretty sure those variables should not be left out.

#63 has some changes that don't seem relevant to this issue, e.g. removal of theme_node_search_admin(). I'll check if any of those are still wanted.

That one came in through #35. I generally agree that we should focus on converting variables to config here and attack the rest in follow-ups.

#63 doesn't always deal with all the variables, e.g. drupal-7.system.database.php only lists 3 of 6, #81 lists 4 of 4.

It is probably important to note, that the list of variables is not exclusive. Contributed modules also may implement hook_ranking and through that mechanism contribute more variables with node_rank_-prefix.

jhodgdon’s picture

Issue tags:+Needs tests

Regarding #84 - yes there are hook_ranking() implementations, but I do not think they have been working since Drupal 7.x. See
#893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken
And it looks like the patch in #81 has an upgrade hook that is converting all node_rank_* variables to config, so I don't think we are losing any information in the upgrade?

Anyway... This patch looks quite reasonable and is a straightforward conversion of variables to config, so I think we should probably just commit it.

But I have two concerns about tests:

a) It's interesting to me that the test for ranking is using a config_set() instead of testing the UI (using the Search settings page). Do we have a test that uses the UI? If not, we should add one.

b) I think I'd like to see a test added with a test module that adds a new type of ranking with hook_ranking(), and verifying that it works correctly. This should have been done in 7.

I realize that both of these tests are a bit out of scope for the issue, but it's hard to verify that the change is working properly without better tests. Thoughts? We could move these to a separate task issue...

Does anyone have any concerns about the proposed patch that would keep it from being committed?

ianthomas_uk’s picture

I've compared #63 and #81 line by line, differences are:
- Changes for #1938920: Convert node_search_admin theme tables to table #type (thanks znerol)
- Config name has changed from node.search to search.plugin.node_search
- Configs are being set one at a time in a loop rather than as an array. I don't see a major difference between the two approaches, the loop is probably slightly safer in case there are already values that would be overridden but I can't see a scenario where that happens.
- The update functions are very different, but should be removed anyway as upgrades will not be supported in Drupal 8.
- $this->nodeSearchPlugin is set in each of the tests. I'm not sure what purpose this serves.
- rank_custom added to test and database upgrade in #81

It is probably important to note, that the list of variables is not exclusive. Contributed modules also may implement hook_ranking and through that mechanism contribute more variables with node_rank_-prefix.

Shouldn't they keep track of those variables in their own config objects?

znerol’s picture

Shouldn't they keep track of those variables in their own config objects?

I'm not sure how this would be possible, given the way NodeSearch::addNodeRankings() is working at the moment:

<?php
if ($ranking = $this->moduleHandler->invokeAll('ranking')) {
 
$tables = &$query->getTables();
  foreach (
$ranking as $rank => $values) {
   
$node_rank = \Drupal::config('search.plugin.node_search')->get('rank.' . $rank);
    if (!empty(
$node_rank)) {
?>

We'd probably need to change the structure returned from hook_ranking to include the configured ranking factors?

ianthomas_uk’s picture

jhodgdon’s picture

Let's keep the changes to a minimum here: switching the node rankings from variables to Config, and keeping it all in the node config. Is there really a problem if it all gets into the node config? If we made it more complicated, I think we'd have to go to a plugin system for rankings, which wouldn't be a bad idea but might be a bit out of scope for this issue.

ianthomas_uk’s picture

StatusFileSize
new5.05 KB
new6.89 KB
PASSED: [[SimpleTest]]: [MySQL] 59,780 pass(es).
[ View ]

Regarding #84 - yes there are hook_ranking() implementations, but I do not think they have been working since Drupal 7.x. See
#893302: Search ranking based on the factor "number of comments" & "No of Page Views" is broken

For the sake of this issue, lets assume that they work, as they are good examples of hook_ranking(). That's a worthwhile followup though.

Shouldn't they keep track of those variables in their own config objects?

I'm not sure how this would be possible, given the way NodeSearch::addNodeRankings() is working at the moment:

I think the "correct" way of fixing this for CMI would be to replace hook_ranking with hooks to add/remove rankings from a list maintained by NodeSearch. A default value could then be supplied when the ranking was added. That's outside the scope of this issue though.

Is there really a problem if it all gets into the node config?

The problems are where do defaults come from (they can't come from a yml or an install hook), and when happens when a module is uninstalled (its config should be removed automatically).

Changes in this patch:
- Upgrade functionality and tests have been removed as that will now be handled by migration.
- All known variables documented in drupal-7.system.database.php in alphabetical order for the benefit of the migration team.
- node_ranking_custom removed as that doesn't seem to be used anywhere (I think it was just there as an example that other node_ranking_* variables could be added).
- Values are now stored as integers.
- I've gone back to creating an array and foreaching when saving the variables. This means that any ranks provided by a hook_ranking() function that has since been uninstalled will be removed from the configuration.
- A default is now provided when building the admin form, as we know there will be instances where the default is not provided in the yml. This isn't best practise, so I've added a comment to stop people copying this coding style.

Test concerns from #85 are still valid but could be addressed in a followup if necessary.

znerol’s picture

+++ b/core/modules/search/lib/Drupal/search/Tests/SearchRankingTest.php
@@ -114,10 +114,13 @@ public function testRankings() {
+      $this->nodeSearchPlugin = $this->container->get('plugin.manager.search')->createInstance('node_search');

There is no need to retrieve the plugin instance multiple times. Just reuse the one instantiated in setUp

Other than that, its fine with me.

ianthomas_uk’s picture

StatusFileSize
new8.51 KB
PASSED: [[SimpleTest]]: [MySQL] 59,393 pass(es).
[ View ]
new4.88 KB
FAILED: [[SimpleTest]]: [MySQL] Unable to apply patch interdiff-90-92.diff. Unable to apply patch. See the log in the details link for more information.
[ View ]

Provided the tests pass, I think this is as good as we're going to get with the current API.

Changes:
- Now aims to store all possible ranks, even if a module is not currently enabled. Previously saving the search settings page would often result in keys being added/removed to the rank array. For example, straight after an install, it would add 'comments' (because the comment module is enabled) and remove 'recent' (because cron has never run). This now saves very slightly more context, but should greatly reduce the chance of config appearing to be different when you've not changed anything.
- Improved the tests to work with whatever happens to be in the ranks array. Previously it assumed comments and views would be first, and it would only disable ranks that it knew about.
- Removed $this->nodeSearchPlugin = $this->container->get('plugin.manager.search')->createInstance('node_search'); lines. They were added in #65 without explanation, and I don't see the need for them.
- Moved the yml into the search module, since it's config for a search plugin (as indicated by the name starting search.).

Once this is committed I plan to work on some of the issues mentioned above, and may even replace hook_ranking.

Status:Needs review» Needs work

The last submitted patch, 92: interdiff-90-92.diff, failed testing.

ianthomas_uk’s picture

Status:Needs work» Needs review

Should probably have given that a txt extension ;)

jhodgdon’s picture

I noticed that the existing SearchRankingTest does not test setting the rankings via the UI. So I don't think we have a test that verifies that this works; in which case, it is difficult to tell whether this patch might break the UI for setting rankings.

Can we add a test for this, or modify the existing SearchRankingTest so that it uses the UI instead of Config/variable_set() to set up the rankings?

jhodgdon’s picture

I noticed that the existing SearchRankingTest does not test setting the rankings via the UI. So I don't think we have a test that verifies that this works; in which case, it is difficult to tell whether this patch might break the UI for setting rankings.

Can we add a test for this, or modify the existing SearchRankingTest so that it uses the UI instead of Config/variable_set() to set up the rankings?

alexpott’s picture

I'm not entirely happy with the approach taken by #92 for a number of reasons. Configuration is supposed to be injected into plugins to reduce dependencies - this is exactly what the previous patches were doing. The rationale for going back to just getting config inside the plugin from #81 was

This seems an odd hybrid of the two, in that it looks like you're passing in configuration, but really you're passing in a reference to the configuration, and that reference is always going to point to the same place! (NodeSearch will always be search.plugin.node_search)

Well the current approach (#92) in fact are a tighter coupling between the config object name and the plugin as it is hardcoded in the plugin.

In #72 it was injected through the plugin manager. This would have allowed a plugin to be created with a reference to any configuration object as long as it was keyed by the plugin name in the configuration array. This meant that the approach allowed the configuration of any search plugin in a consistent way. Yes it is different from the config entity approach but that was deliberate since the overhead is unnecessary since there will be a 1 to 1 relationship between plugin and config object.

Furthermore if we are going to go down the approach taken in #92 then the config factory has to be injected. But I really hope we do not.

Yes the hook_ranking is weird and complicates the patch but that is not solved by either approach.

ianthomas_uk’s picture

if we are going to go down the approach taken in #92 then the config factory has to be injected.

Do you mean #92 still does this for search.settings? If so, that's #2123073: Move index.cron_limit setting to NodeSearch

Or do you mean that we need to change #92 to inject search.plugin.node_search instead of calling \Drupal::config() from the plugin? If so, is there any documentation about when you should use \Drupal::config() and when you should inject configuration?

jhodgdon’s picture

So... I'm a bit confused.

The Search Settings page is allowing individual plugins to add settings to the page, which seems logical. Node search ranking "weights" are an example of this.

They need to be saved somewhere -- either the plugin needs to save them in configuration, or they need to go into the generic search settings configuration. Which do we think makes more sense? Whichever it is, the NodeSearch plugin at some point will need to retrieve the weights that the user has set, to form the search query.

So let's step back and make that first decision: should search plugins have their own config for their specific settings, or use the central search settings? And then we can decide how best to retrieve them (where to inject etc.). The latest patch puts them in central search settings... I can't tell whether alexpott thinks that is a good idea or not?

alexpott’s picture

Plugins should definitely have their own settings. As these plugins are only active when the search module is eanbled their config file should begin with "search.". Personally I think that this configuration should be injected into the plugins by the search plugin manager - in that way we can document and enforce a standard practice for all search plugins and all of them we will have names that make sense.

@jhodgdon afaics no patch here has tried to save the configuration to search.settings.yml

jhodgdon’s picture

What I meant in #99 was to ask whether node.module should be handling the configuration for the NodeSearch plugin, or whether it should be part of search.module's config handling. I'm not sure I made myself clear there? Maybe.

Anyway, it sounds like #100 is advocating that all configuration from other modules' search plugins should be gathered by search.module, stored in search.module's configuration (search.settings.yml), and injected in a standard way (maybe in the constructor/create method?) into search plugins so that they have it available when they need it.

If I've correctly grasped #100, I think this is a good idea and it sounds like a good pattern to follow. Probably we'd need a method or two added to the SearchPluginBase class and the SearchPluginInterface (maybe?) to deal with this, so that the SearchPluginManager class could properly do the injection?

And I agree that none of the patches I've seen on this issue are doing this.

So if I've understood correctly... we'd have a section called maybe search.plugin.node_search or search.plugin.user_search, and the specific plugin could put anything it wants into that namespace for configuration.

Then during plugin instantiation, the SearchPluginManager could grab the search.plugin.[plugin id] section of search.settings configuration, and pass that in to the plugin to use as they see fit. There would also need to be a way for the plugin to give the settings to the search plugin manager to store, after the user updates them via the Search Settings Page or perhaps a plugin-specific config page.

Or something like that?

ianthomas_uk’s picture

I can see arguments on both sides of this one.

Even if we enforce that a search plugin's settings are stored under search.plugin.*, the search can't specify the schema inside that or even what the valid values of * are, so I wonder what the benefit is. What do we do for plugins that don't actually have any configuration of their own? Should they ship with their own, empty, search.plugin.my_plugin.yml files?

Other than to expose an admin UI, there doesn't seem to be a need for plugins to expose that they support configuration, so why not encapsulate all configuration inside the plugin? (i.e. move back to node.search.yml). It doesn't seem like it needs to be part of the plugin API to me, although certain plugins may want to add their own methods to expose specific configuration (not that I can think of any examples).

@jhodgdon anything under search.* belongs to the search module. I think search.settings should only contain config from the search module directly, not extensions of the search module.

There's a related question in #2102521: Finish converting menu.module to CMI - what's the best way to provide defaults when you don't know the config structure beforehand?

alexpott’s picture

@jhodgdon I was not suggesting that the node module's search plugin configuration defaults should be provided by the search module - the search.plugin.node.yml should definitely by in the node module's config folder. This means it'll only by installed when both the node and search module are enabled. Which is exactly what we want since this configuration only has meaning when both modules are enabled.

If the plugin has no configuration then there does not need to be a file - empty config files make no sense.

The schema for each plugin's config file if it has one should be provided by the module that provides the plugin and default config (in this case node).

@jhodgdon the patch in #72 is the last patch to inject config into the plugin using the search plugin manager. Which I thought was quite close to rtbc.

jhodgdon’s picture

Ah, OK. Thanks for the extra explanation.

ianthomas_uk’s picture

I agree search.plugin.node_search.yml should be moved back to the node module so that it is there if and only if node and search are both enabled (although it will remain if first both are enabled, and then node is disabled).

I’m still not convinced that the config needs to be injected rather than encapsulated though.

Injection enforces consistent naming across all search plugins, but means you can’t search the source code for those names (grep search.plugin.node_search will return no results) and doesn’t enforce anything for plugins of other modules. Instead, I propose we document the convention and ensure it is consistently used across core, so contrib modules copy it. We’re already doing this with the module.settings convention, and even the convention that config names start with the module name.

Configuration is supposed to be injected into plugins to reduce dependencies

Is this a coding standard? What dependencies are removed by injecting configuration?

If we are going to go down the approach taken in #92 then the config factory has to be injected.

Why is this required? Can’t we just use \Drupal::config()?

Injection allows a plugin to be created with a reference to any configuration object as long as it is keyed by the plugin name in the configuration array.

When would we have multiple configuration objects with the same keys? Is this for unit testing or something? The plugin will expect defaults to be provided, and we will are only doing this with one file, which has a “hard coded” file name (search.plugin.node_search.yml)

If the plugin has no configuration then there does not need to be a file - empty config files make no sense.

With the approach in #72, you’d still be creating an empty config object to inject though.

ianthomas_uk’s picture

I had a chat with alexpott on IRC, to summarise we shouldn't call \Drupal::config() from a class because it makes unit testing harder, so we need to inject the config (ala #72). However, to make the patches simplier, it may be easier to provide the config object first in a separate issue, then add a follow up patch here to start using it.

Relevant chat text:
alexpott>the thing is we should have a single way to configure search plugins that's why moving the config loading to the manager makes sense
alexpott>so what we could do is split the patch into 2 issues... get the plugin config management right
alexpott>and then convert node ranking form variables
alexpott>perhaps that way it'll be easier to grok
...
alexpott>the search for the yml name is moot... try searching for a config entity name.
ianthomas_uk>I think to sum up, you're enforcing the config name, but not the schema or defaults.
alexpott> if we handle the injection correctly (as no patch has done this yet) we should be able to remove the plugin's dependency on the configuration system
ianthomas_uk>Searching certainly doesn't always work, but when it does it can be a big benefit. Particularly for people less familiar with the code. Not a huge issue though.
alexpott> no we should not use \Drupal::config() in classes
alexpott> it means that unit testing them - when we get round to it :) - it more difficult than it should be
alexpott> see https://portland2013.drupal.org/session/dependency-injection-drupal-8
ianthomas_uk>OK, I can see benefits in that policy, but mainly if applied consistently
alexpott>True we will be creating an empty config object with the current approach but actually that is avoidable
alexpott>we are trying to apply it consistently it has just been a challenge :)
alexpott>lots and lots of chicken and egg problems

ianthomas_uk’s picture

Status:Needs review» Postponed

@jhodgson I've added a test in #2132647: Add test for the settings form of the NodeSearch plugin, so it can get committed while we're working out what to do here.

I've also split injecting configuration off into #2132649: Inject a configuration into SearchPluginBase and postponing this issue on that one.

xjm’s picture

Issue tags:+beta blocker
xjm’s picture

tim.plunkett’s picture

Status:Postponed» Needs work
Related issues:+#2132649: Inject a configuration into SearchPluginBase

I'm rather worried about how #2132649: Inject a configuration into SearchPluginBase will actually turn out, can we not resolve this critical/beta blocker with \Drupal::config() and work through the other issue at it's own pace?

alexpott’s picture

Status:Needs work» Postponed

#110 I don't think so as the final solution probably will affect the configuration file name and maybe the structure of the data. So I agree with postponing this issue on that - thanks @ianthomas_uk for doing the issue queue work.

alexpott’s picture

ianthomas_uk’s picture

Status:Postponed» Closed (duplicate)
xjm’s picture

Issue tags:-beta blocker