Updated: Comment #43

Problem/Motivation

The Views module in D7 and earlier versions integrated some from the core search module, which could be used as filters for Node-based views:

a) Search keywords (you could filter by keywords indexed by search.module with contextual and regular keywords)

b) Node-node links (on a view of nodes, you could filter by nodes that link here, and nodes that this node links to, with contextual and regular filters).

This issue is about deciding which of these needs to be restored and then doing it.

Proposed resolution

a) (definitely) Restore the search keywords filters. There's some code in Core to do this, but isn't currently working, since it is making incorrect assumptions about the search tables. Needs tests also.

b) (needs opinion from catch or Dries) (probably not) Restoring the node links filters is more problematic. In D7, the data on "node A links to node B" was calculated during search indexing and stored in the search index. However, when search was convered from a hook to a plugin system, the code that did this was removed from the search module. This was done because:
- The code was very node-specific but was being applied to all search indexing (inappropriately).
- It was really terrible code that wasn't compatible with multi-lingual nodes and wasn't very smart about finding links either.
- It wasn't really adding anything useful to the problem of making a good search index.

If we did want to do this, we would need to create the backlinks table during Cron runs. We would basically need to take the code from the D7 search indexing logic, plus or minus a bunch of bugs that it currently has, fix it to make it actually work correctly, and make a hook_cron to generate the table of links. The steps are:
- When a node is created or updated, note that it needs to be scanned for links. This is recorded in a new table {node_links_queue}.
- During cron, queue up nodes that need updates that are not yet queued.
- When a node is deleted, remove it from the links tables.
- Let the cron system run the queue during cron runs. Each queue entry will:
--- Render the node, in all languages it has
--- See if the text has any links in it [use PHP's DOM classes to parse for links]
--- See if those links are to nodes [this is the hard part: trying to figure out if a given a HREF tag is a link to a node, given that it could be aliased and given that URLs are language-dependent. D7 code for this is rather broken. We need a better system]
--- If there is a link in any language from one node to another, save this in the new {node_links} table.

And then we need a hook_views_data to reference this table, similar to whatever is in the D7 views module for this table.

Remaining tasks

1. Agree on the approach (which functionality are we doing and which are we dropping).

2. If we are doing node-node links, we also need to decide if the algorithm we had in D7 is OK to use or if it needs an update (it uses a regular expression to find links rather than a DOM analysis, and it doesn't consider languages). Fixing it to work better could be a separate issue.

3. Get the patch working, with tests.

User interface changes

Node links (maybe) and search keywords (definitely) will be available in Views as filters.

API changes

The node links table will exist (maybe) -- different name from D7 though. Node links (maybe) and search keywords (definitely) will be available in Views as filters.

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

xjm’s picture

Status: Needs review » Needs work

Whoops, wrong patch.

xjm’s picture

Status: Needs work » Needs review
FileSize
3.86 KB
xjm’s picture

This was probably supposed to also include the backlinks default view (?) but that didn't get removed when the data integration did. So that View should be reviewed with this as well.

Status: Needs review » Needs work
Issue tags: -VDC

The last submitted patch, search.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
Issue tags: +VDC

#2: search.patch queued for re-testing.

dawehner’s picture

FileSize
3.36 KB

Let's clean up the search.views.inc file.

aspilicious’s picture

Status: Needs review » Needs work
Issue tags: +Needs tests

This looks great. This even fixes a core bug. The default view "backlinks" is totally broken without this one.
So we need tests for this...

dawehner’s picture

Status: Needs work » Needs review
FileSize
7.18 KB

Added a test for the backlinks view and moved it to search module.

damiankloip’s picture

This looks good to me, I guess eventually the DefaultViewsTest class we be gone.

+++ b/core/modules/search/lib/Drupal/search/Tests/Views/BackLinksViewTest.phpundefined
@@ -0,0 +1,44 @@
+ * Tests the backlinks view.

default view ?

+++ b/core/modules/search/lib/Drupal/search/Tests/Views/BackLinksViewTest.phpundefined
@@ -0,0 +1,44 @@
+      'description' => 'Tests the backlinks view.',

Same here.

Maybe not, but just an idea.

dawehner’s picture

FileSize
7.21 KB
1023 bytes

I guess eventually the DefaultViewsTest class we be gone.

yeah we should write proper test cases for all of them in the longrun, as this only really takes care that they work as expected.

Status: Needs review » Needs work
Issue tags: -Needs tests, -VDC

The last submitted patch, drupal-1853536-10.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review

#10: drupal-1853536-10.patch queued for re-testing.

dawehner’s picture

Issue tags: +Needs tests, +VDC

#10: drupal-1853536-10.patch queued for re-testing.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs tests

This always can use more tests. But this patch actually fixes some core bugs. I don't want duplicates to be created so lets commit this.

xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs tests

http://drupal.org/core-gates#testing

I think we at least want some unitish tests for the hook_views_data(). I agree that we can clean up the default view tests in a followup; there may even already be an issue for this somewhere.

xjm’s picture

Title: Reintroduce Views integration for search.module » Reintroduce Views integration for search.module and fix the backlinks view
Category: task » bug

Per #7 and #14.

dawehner’s picture

Issue tags: -Needs tests
FileSize
21.26 KB

Yeah I agree we better have some sort of better test coverage now, but I agree with aspilicous, there is always room for more.

If and once we replace the /search page with a view, the specific test coverage would be way way better.

dawehner’s picture

FileSize
21.26 KB

Even issues like that can get a rerole :)

dawehner’s picture

FileSize
1.62 KB

I think an interdiff could be helpful here.

aspilicious’s picture

Status: Needs review » Reviewed & tested by the community

Lets try again...

catch’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/search/lib/Drupal/search/Plugin/views/argument/Search.phpundefined
@@ -110,6 +112,9 @@ public function query($group_by = FALSE) {
+
+      // Set to NULL to prevent PDO exception when views object is cached.
+      $this->search_query = NULL;

I don't get this, why's there a PDO exception if the views object is cached? The comment should explain this more.

dawehner’s picture

Status: Needs work » Needs review
FileSize
1.52 KB
21.72 KB

What about

      // Set to NULL to prevent PDO exception when views object is serialized in
      // form cache.
dawehner’s picture

Issue tags: -VDC

#22: drupal-1853536-22.patch queued for re-testing.

dawehner’s picture

#22: drupal-1853536-22.patch queued for re-testing.

Status: Needs review » Needs work
Issue tags: +VDC

The last submitted patch, drupal-1853536-22.patch, failed testing.

dawehner’s picture

Status: Needs work » Needs review
FileSize
21.42 KB

Just another rerole.

Status: Needs review » Needs work

The last submitted patch, drupal-1853536-26.patch, failed testing.

dawehner’s picture

Priority: Normal » Major

Bumping to major as this would be a clear regression.

jhodgdon’s picture

I've just filed #2083717: Convert Search Results to Views which is a bit more ambitious than this proposal. I plan to work on it there...

But this one is also apparently about the backlinks view, so I guess I will leave it open.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon

Also since it wasn't noted here before, #2003482: Convert hook_search_info to plugin system removed the backlinks table since it wasn't needed by Search itself, and the backlinks view. Comment #2003482-95: Convert hook_search_info to plugin system discusses what can be done for the backlinks view.

I think I'll assign this to myself. It doesn't look like it's been worked on lately and it likely needs more work since #2003482: Convert hook_search_info to plugin system went in.

jhodgdon’s picture

Category: bug » task

This is actually a task, not a bug?

dawehner’s picture

Well, it is a regression compared to d7.

dawehner’s picture

Well, it is a regression compared to d7 and could also block people from upgrading.

jhodgdon’s picture

Category: task » bug
Issue tags: +Regression

OK

jhodgdon’s picture

Issue summary: View changes

Removing myself from the author field to unfollow the issue. --xjm

jhodgdon’s picture

Created an issue summary.

jhodgdon’s picture

Assigned: jhodgdon » Unassigned
Status: Needs work » Needs review
Related issues: +#2003482: Convert hook_search_info to plugin system
FileSize
8.39 KB

OK. Here's a first pass at getting this actually working. It is a complete, working patch, aside from the fact that it has no tests, but I have tested it manually.

What it does:
a) Creates a "node_links" queue for cron.
b) If you create or update or delete a node, the patch to the Node.php entity controller adds a queue entry to the "node_links" queue.
c) When one of those queue entries is run, it renders the node in Search Index view mode, uses DOM to find links in the rendered content, and saves all entries that appear to be links to node/#### in the node_links table.
d) Views is made aware of this data. The Node base table has filters and arguments to filter by nodes linked to and nodes linked from.

Let the bikeshedding begin!

Some things that probably still need to be done:

1. Probably needs a test. As far as I know, there never was a test. The test could make several nodes and use various methods to add links in the body (alias, bare node ID, with http:, starting with base_path(), etc.). Then it would need to run cron to generate the links table, and make views that would use filters/arguments to see the forward and back links. This is probably beyond what I can do right now with Views tests -- hopefully someone is more familiar with them than I am?

2. I'm not sure what to do during a 7 to 8 upgrade. Prior to the conversion of Search to plugin system, when we dropped the {search_node_links} table that this replaces, the 7 to 8 upgrade truncated this table. But then the expectation was that during search indexing (search index tables were also truncated), the node links would be regenerated. I guess we need to do something else to make sure the nodes get updated, although if during update Node->save() is called, the job will be queued.

3. I'm not sure using a queue is the best idea but I kind of like queues. Nodes could be queued multiple times though... it may be useful to keep a table of "queued for linking" and only add a node to the queue if it is not already in the queue, and then when the queue job runs, remove that node from the "queued for linking" table?

Anyway... the code is there for at least making the links. I think at this point I might un-assign myself and let someone else take it from here?

Status: Needs review » Needs work

The last submitted patch, 36: 1853536-working-node-links.patch, failed testing.

jhodgdon’s picture

Oh, sigh. This only takes care of the backlinks, not the rest of the search module integration. It really isn't a search module issue at all, except for the fact that in D7 the backlinks table was part of search module.

jhodgdon’s picture

Whoops. I had tested the install with a previous version and had a paste error when I updated it, missing paren. Hopefully it will work this time.

Status: Needs review » Needs work

The last submitted patch, 39: 1853536-working-node-links-hopefully.patch, failed testing.

jhodgdon’s picture

Well, that's interesting.

Most of the test failures are due to the {queue} table not being enabled during various tests. Not sure why. Maybe we can't or shouldn't queue stuff from the Node controller? I guess the patch probably needs to keep another table of which nodes need updating, and during node save/delete just update that table, and then in hook_cron(), add stuff to the queue. Locale is doing something like that with its queue.

There was also one exception in the DOM where it got an empty string to parse. I guess the patch needs to test for that before it calls loadHTML(), and just do continue() if that occurs.

dawehner’s picture

@jhodgdon

Can't we split up this patch to at least get the normal views integration back in? I would consider it as a big regression if you cannot upgrade a view from d7 to d8 as we dropped the entire support of views.

jhodgdon’s picture

Assigned: Unassigned » jhodgdon
Issue summary: View changes

The patch I posted actually doesn't include the previous search integration at all. It was just the backlinks. So yes, it will be easy to split it up, just go back to a previous patch and get that working. :)

So... I had a few more thoughts about the backlinks part. Basically, it was a straight port of how the backlinks checking was working in D7, except that instead of doing it during search indexing, it's doing it in a queue.

However. It. Has. Some. Problems.

The major one is that it's using a really stupid way to figure out if there is a link from one way to another. In D7, it actually used a regular expression to parse for <a href=; in the D8 port, at least it's using DOM to do the basic HTML parsing.

But after that, the code is the same. Once you figure out you have an A link with an HREF attribute, you have to figure out "Is that a link to a node, or to something else?". The d8 code proposed in the previous patch is the same as the d7 code, and uses a regular expression to try to figure this out. But there is no way that regular expression is very good, and it's definitely not respectful of languages. Then it tries to look up the alias and again is not language-aware on that.

So we really need a better way to say "given an HREF, is it a link to a node, given the language of this version of the node" rather than a straight port of the broken code from D7.

And then I think the queueing system needs some work too. Probably we need to add two columns to the Node table: links_queued and links_need_update, and in the update function, we'd set queued to FALSE and need_update to TRUE. During Node->afterSave() or whatever that method is, we would set links_need_update to TRUE. During Node->delete() or preDelete() or whatever, we would delete the links record directly. During cron, we would scan for nodes with links_queued FALSE and links_need_update TRUE, and add them to the queue. And then cron would take care of running the queue as needed. (I do think using a queue is a better idea than running a certain number directly during cron, since it has the idea of timing built in and there are also other ways to run the queue.)

And finally, I think we should not save language in the node_links table -- we should just save the connection between nodes if it's linked in any language. The views integration doesn't consider language anyway (or at least it didn't in D7 and I think it's not necessary -- hopefully the links would exist in all languages anyway.)

Anyway.

So yes, let's go back to the basic search integration and get that working first, and think about a way to solve the "figure out if it's a link to a node in text of a given language" problem, because without that actually working, this whole backlinks thing is kind of flawed.

I'll assign back to myself and do another pass, this time with the basic search integration. And I'm updating the summary with these thoughts.

jhodgdon’s picture

I guess another question is whether all of this Node Links functionality should be an always-on function of the Node module or put into a separate core (or even contrib) module? I am actually leaning towards contrib. How many people actually want this for their Views?

jhodgdon’s picture

I also took a look at the patch in #26, which was for the basic search integration.

It also has some problems:
- There seems to be an assumption that the search index table will join to a Views base table, and that the previously-module, now-plugin column in the search index table will match the name of the views base table. While that happened to work in D7, it will certainly not work here.

-      $this->search_query->searchExpression($input, $this->view->base_table);
+      $this->search_query->searchExpression($input, $this->view->storage->get('base_table'));

The column in the search table is now the name of the plugin, which is node_search.

- Really, the whole idea of search integration for Views has to be done on a search plugin basis, and here we should only be doing it for the node search plugin. We can have a base class in Search module that lays out the framework for modules that provide search via the search index, but the actual integration should be done in conjunction with the module that provides the search plugin (in this case, NodeSearch). So the hook_views_data() should be in node, not search.

Anyway... I think I can work on a patch, but it's going to be pretty complicated.

jhodgdon’s picture

Status: Needs work » Needs review
FileSize
32.16 KB

OK, here's another patch: I included the search keywords integration as well as the node links this time, and I fixed up the links with another table to keep track of queueing.

In my manual testing, it's all working pretty well except:

a) I am unable to add the Search Keywords to a view using the Views UI. I have no idea why. I can see it in the list but when I click "add and configure" it just comes back to the filter list, and I can click "add and configure" again and it comes back... I can never seem to add it and there are no messages in my log. The keywords Argument plugin works fine as a filter, just not the Filter plugin.

b) The Search Score field doesn't output anything. I tried the query that Views reports it is using and that works fine, but the field does not output any text.

c) As before, I think the algorithm used to find links in nodes is stupid. I've moved it into its own function this time and put a bunch of @todo comments in it.

d) I didn't attempt to include the tests from the patch in #26, but I think I got the gist of the rest of the #26 patch into this one.

Since this patch is a combination of two I'm not sure an interdiff is useful, so I didn't create one. Sorry.

Status: Needs review » Needs work

The last submitted patch, 46: 1853536-search-views-46.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
33.74 KB
2.73 KB

So I think you couldn't add the handler as the autoloader was unhappy that the class was called SearchFilterBase but the file was Search.php. The ajax 'stuff' was just getting in the way of seeing that (Not good for debugging).

And with the hidden field; If you see the filters Score::query method:

// Hide this field if no search filter is in place.
$this->options['exclude'] = TRUE;

We will get to here whenever search_score is not set on a filter handlers, which could be broken handler (as you found out), no keywords, or filter is exposed and no input has been entered. So I guess we don;t really want to do that? It seems extreme to hide the whole field based on this? it should maybe just output NULL or something? What do you think?

Hope this helps!

Status: Needs review » Needs work

The last submitted patch, 48: 1853536-48.patch, failed testing.

damiankloip’s picture

Status: Needs work » Needs review
FileSize
36.07 KB
7.15 KB

Ah, spotted your @todo about making the search plugin dynamic. We can pass this in from the views_data, and then use it from there.

It seems that generally the whole search plugin system needs alot of work.

jhodgdon’s picture

RE #50 - you think that the search plugin system in general needs a lot of work, or the Views part of it?

Anyway... The interdiffs look excellent! Thanks for figuring out what I was trying to say about the search plugin being passed in. I kind of sort of knew how to do this in Views 7 but wasn't sure in 8. And there's no documentation anywhere that I could find. :(

So... In your opinion, what still needs to be done to this, besides getting in some tests for the search filters, fields, etc.?

jhodgdon’s picture

50: 1853536-49.patch queued for re-testing.

The last submitted patch, 50: 1853536-49.patch, failed testing.

jhodgdon’s picture

Status: Needs review » Postponed
Issue tags: +Needs reroll

Not sure why the test bot didn't update the status...

It looks like this issue needs a reroll (not surprising). And we probably should wait until
#2042807: Convert search plugins to use a ConfigEntity and a PluginBag
gets in, as it is really reworking the search plugin system.

jhodgdon’s picture

Status: Postponed » Needs work

Other issue is taken care of.

jhodgdon’s picture

OK, this needed some reworking because of #2042807: Convert search plugins to use a ConfigEntity and a PluginBag, and a reroll; an interdiff is not unfortunately possible, but here are the changes needed (I'm working on a patch and will test it before I post it here):

a) The code in node.views.inc that was trying to determine if Node Search was enabled (only join the Search tables if so) had to change, because instead of plugins being activated, now you create search pages of the given plugin type.

b) For the generic Views argument Search handler, rather than using the plugin type, what we really want to do is make it take the search type... how to explain this... So basically, plugins that use the core Search index have a choice of putting whatever they want into the 'type' column of the Search db tables. NodeSearch uses 'node_search' as the type, and even if there are multiple NodeSearch pages, they share one index. However, other search plugins may not use the plugin ID. So I changed the argument handler so it explicitly says you need to specify the 'type' column rather than assuming it's the plugin ID.

c) Same as (b) for the Search filter handler.

d) There is something in the latest patch for core/modules/views_ui/lib/Drupal/views_ui/Form/Ajax/ViewsFormBase.php that seems totally unrelated to this issue so I'm taking that out.

Patch coming soon...

jhodgdon’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
48.33 KB

OK, here's a patch.

In addition to what I said in the last comment, I also added a test. It tests the search keywords filter (just with one keyword), the search keywords argument (contextual filter), the "node links to" filter, and the "node is linked to" filter. I also tested them manually (that is how I made the test view), and they all appear to be in working order.

I think this is ready to go, unless we need more tests, but at least the basic functionality is there, tested, and working. Let's see if the patch breaks anything else and if the tests pass on the test bot.

jibran’s picture

Some minor issues,

  1. +++ b/core/modules/node/lib/Drupal/node/Entity/Node.php
    @@ -152,12 +152,23 @@ public function postSave(EntityStorageControllerInterface $storage_controller, $
    +      $nid = $entity->nid->value;
    

    Why not $entity->id();?

  2. +++ b/core/modules/node/node.module
    @@ -2087,3 +2096,168 @@ function node_comment_unpublish($comment) {
    +        // Record that we found a link. We only want to store one
    +        // link to this node, and only if it's not a self-link.
    

    We can expand this comment.

  3. +++ b/core/modules/search/lib/Drupal/search/Plugin/views/argument/Search.php
    @@ -64,18 +89,12 @@ public function query($group_by = FALSE) {
    +      $join = \Drupal::service('plugin.manager.views.join')->createInstance('standard', $definition);
    

    We can inject this service.

  4. +++ b/core/modules/search/lib/Drupal/search/Plugin/views/filter/Search.php
    @@ -90,32 +114,28 @@ public function validateExposed(&$form, &$form_state) {
           $this->search_query = db_select('search_index', 'i', array('target' => 'slave'))->extend('Drupal\search\ViewsSearchQuery');
    

    We can also inject database connection.

  5. +++ b/core/modules/search/lib/Drupal/search/Plugin/views/filter/Search.php
    @@ -137,26 +157,21 @@ public function query() {
    +      $join = \Drupal::service('plugin.manager.views.join')->createInstance('standard', $definition);
    

    We can inject this service.

  6. +++ b/core/modules/views/lib/Drupal/views/Tests/SearchIntegrationTest.php
    @@ -0,0 +1,90 @@
    + * Definition of Drupal\views\Tests\SearchIntegrationTest.
    

    Contains

  7. +++ b/core/modules/views/lib/Drupal/views/Tests/SearchIntegrationTest.php
    @@ -0,0 +1,90 @@
    +    $nodeURL = $this->getUrl();
    

    $node_url

jhodgdon’s picture

Thanks jibran!

I am not sure how to make the injection stuff work and I don't think any of the other Views filters do that, so I'm not sure how to do it. If you can get it working and make the new test added in this patch pass in your own environment, that will be fine. Otherwise, I'd say it is out of scope since the patch is not adding these injections (two of them are slightly modified lines of code, but the service injection was already in there before this issue -- before the patch it just made the same call in a slightly different way).

The coding standards and other minor fixes -- sure, let's make those changes. Again, if you'd like to make a new patch, great -- otherwise I can take care of this.

I'm not sure what you would like to see in item 2 though for an expanded comment... I thought the comment was pretty clear and complete.

jhodgdon’s picture

jibran clarified in IRC that item 2 means the line length for that comment should be closer to 80 characters. True!

jhodgdon’s picture

FileSize
48.38 KB
2.63 KB

OK, here's an update for the coding standards stuff from jibran/#58 I did not address the injection stuff. If someone else wants to tackle that, great! But I think it's out of scope for this issue, since this patch didn't introduce it.

jhodgdon’s picture

Issue summary: View changes

Updated issue summary with current proposal/tasks.

dawehner’s picture

  1. +++ b/core/modules/node/node.install
    @@ -416,6 +416,60 @@ function node_schema() {
    +  );
    ...
    +  );
    ...
     
    +  $schema['node_links'] = array(
    +    'description' => 'Stores links from one node to another node',
    +    'fields' => array(
    +      'source_nid' => array(
    +        'type' => 'int',
    +        'unsigned' => TRUE,
    +        'not null' => TRUE,
    +        'default' => 0,
    +        'description' => 'The {node}.nid of the node containing the link.',
    +      ),
    +      'link_nid' => array(
    +        'type' => 'int',
    +        'unsigned' => TRUE,
    +        'not null' => TRUE,
    +        'default' => 0,
    +        'description' => 'The {node}.nid that this node links to.',
    +      ),
    +    ),
    +    'primary key' => array('source_nid', 'link_nid'),
    +    'indexes' => array(
    +      'source_nid' => array('source_nid'),
    +      'link_nid' => array('link_nid'),
    +    ),
    ...
    +
    +  $schema['node_links_queue'] = array(
    +    'description' => 'Stores information about whether node links need updating',
    +    'fields' => array(
    +      'nid' => array(
    +        'description' => 'The {node}.nid of the node.',
    +        'type' => 'int',
    +        'unsigned' => TRUE,
    +        'not null' => TRUE,
    +      ),
    +      'needs_update' => array(
    +        'description' => '1 if the links need to be updated for this node.',
    +        'type' => 'int',
    +        'not null' => TRUE,
    +        'default' => 1,
    +      ),
    +      'queued' => array(
    +        'description' => '1 if the link update has already been queued for this node.',
    +        'type' => 'int',
    +        'not null' => TRUE,
    +        'default' => 0,
    +      ),
    +    ),
    +    'primary key' => array('nid'),
    +    'indexes' => array(
    +      'needs_update' => array('needs_update'),
    +      'queued' => array('queued'),
    +    ),
    

    To be honest it is odd that we don't at least allow the schema to be generic for multiple entity types, would you be okay that I work on this?

  2. +++ b/core/modules/node/node.install
    @@ -1157,6 +1211,65 @@ function node_update_8021() {
    +function node_update_8022() {
    +  db_create_table('node_links', array(
    

    Do we really add new update functions these days? Afaik migrate will solve it for us.

  3. +++ b/core/modules/node/node.module
    @@ -2087,3 +2096,168 @@ function node_comment_unpublish($comment) {
    +function _determine_link_to_node($href, $language) {
    

    Urgs. At least we should prefix with "node".

  4. +++ b/core/modules/node/node.module
    @@ -2087,3 +2096,168 @@ function node_comment_unpublish($comment) {
    +    $path = Drupal::service('path.alias_manager')->getSystemPath($match[1]);
    

    This should use path.alias_manager.cached instead.

  5. +++ b/core/modules/search/lib/Drupal/search/Plugin/views/filter/Search.php
    @@ -137,26 +157,21 @@ public function query() {
    +      $join = \Drupal::service('plugin.manager.views.join')->createInstance('standard', $definition);
    

    We can use Views::getPluginManager('join') instead as a shortcut.

jhodgdon’s picture

Regarding the update function, yes we need update functions to make new database tables, add fields, etc. Migrate does not take care of that.

Regarding the table schema being specific to nodes... If you want to make it less specific, that is fine with me, but in that case you will need to move it out of the node module -- if it's in the node module it should be node-specific. The code that finds the links is very node-specific... but sure if you want the table to be more generic in the table, I wouldn't object.

Regarding the rest of the changes, they all seem good (oops on the name of that _ function that didn't start with node!).

And I am happy to let someone else take this issue over. Please. :) At least I think you have a working starting point.

jhodgdon’s picture

Regarding the update functions (#63 item 1)...

It turns out I was wrong and until some later date (which might be Beta or RC or ??? -- not yet determined) we are not supporting 8->8 database updates (and we've decided never to support 7 to 8 database updates).

So we do not need the hook_update_N() here to update the database schema.

jhodgdon’s picture

Assigned: jhodgdon » dawehner

Assigning to dawehner since he foolishly asked if I would mind if he worked on it. :)

dawehner’s picture

So yes, let's go back to the basic search integration and get that working first, and think about a way to solve the "figure out if it's a link to a node in text of a given language" problem, because without that actually working, this whole backlinks thing is kind of flawed.

The more I think about it, the more I have the feeling that this really belongs into a contrib module. If sites want to have such advanced search functionality they will use the new search_api (or how it is called) which will probably build a generic solution then.

@jhodgdon
Sorry, would you be okay with not including it into core?

Nick_vh’s picture

I do think it needs to be in core. We have views in core and the most basic implementation should also be in core. It can't be too hard and the mysql query can be horribly, but at least it will provide basics to Drupal Core.

But I agree Search API or whatever will be the D8 contrib version of core will be improving this vastly and might make this a null-operation. As long as we can make Search in Drupal Core flexible enough so that contrib can override everything but still use some of the configs and routing logic it'd be fine removing all other clutter.

Somehow it looks like I'm contradicting myself, but I believe the search in views using the search tables makes a lot of sense for basic Drupal sites, it just depends how much effort it'd be to actually pull this off.

jhodgdon’s picture

I am fine with the backlinks stuff not being included in core. I was not the person who insisted that it was necessary to put this in core, and it has nothing to do with Advanced Search. It has to do with providing a filter to Views that allows you to filter (or contextual filter) based on node A linking to node B.

We removed the backlinks stuff from search.module because it wasn't really working and didn't really add anything useful to the search module indexing. The "wasn't really working" part has been true for ages in the contrib Views module and core Search, and I agree that putting it in core is problematic.

I don't think we should remove Views search integration from core though. That part is working fine I think here and there is no reason to not have it. The only part that is a problem is backlinks.

dawehner’s picture

We talked about that in IRC and we basically agreed on the point that we should have a search views integration but don't need the backlinks feature in core. This is a really edgecase feature.

dawehner’s picture

Assigned: dawehner » jhodgdon

assign back.

jhodgdon’s picture

Title: Reintroduce Views integration for search.module and fix the backlinks view » Reintroduce Views integration for search.module (not supporting backlinks view)
Assigned: jhodgdon » catch
Issue summary: View changes

I discussed this with dawehner and xjm in IRC today.

dawehner and I both think that the appropriate thing to do here is to make sure that search itself is integrated into Views (so you can use the search keywords filter), but to drop support of backlinks (so you do not have a filter for saying "node A links to node B"). We both think the backlinks filter is not a huge loss, especially since the code in D7 and previous versions that calculated the links is really terrible code, won't work with multi-lingual nodes, and will be difficult to maintain.

I'm updating the issue summary with this idea, but we thought we might need a core committer to sign off on it, so I'm assigning this to catch (please reassign if someone else would be more appropriate).

jhodgdon’s picture

FileSize
31.25 KB

Here's a new patch that only does the search keywords filter integration for Views, leaving out the backlinks part. It's the same exact patch as in #61 except that a lot of it has been removed.

dawehner’s picture

It is great to see actual test coverage here as well. There are just a couple of nitpicks left.

  1. +++ b/core/modules/search/lib/Drupal/search/Plugin/views/argument/Search.php
    @@ -19,25 +21,48 @@
    +  function queryParseSearchExpression($input) {
    
    +++ b/core/modules/search/lib/Drupal/search/Plugin/views/filter/Search.php
    @@ -90,32 +114,28 @@ public function validateExposed(&$form, &$form_state) {
    +  function queryParseSearchExpression($input) {
    

    If we change this line already let's mark the issue as protected.

  2. +++ b/core/modules/search/lib/Drupal/search/Plugin/views/argument/Search.php
    @@ -64,18 +89,12 @@ public function query($group_by = FALSE) {
    +      $join = \Drupal::service('plugin.manager.views.join')->createInstance('standard', $definition);
    ...
    -      $join = \Drupal::service()->get('plugin.manager.views.join')->createInstance('standard', $definition);
    

    Note: you can also use \Drupal\views\Views::pluginManager('join' instead.

  3. +++ b/core/modules/views/lib/Drupal/views/Tests/SearchIntegrationTest.php
    @@ -0,0 +1,80 @@
    +
    +  public static function getInfo() {
    

    Missing {@inheritdoc}

jhodgdon’s picture

FileSize
31.33 KB
3.33 KB

OK on 1 & 2, new patch attached.

On 3 I don't think we do that on getInfo() methods on tests. Sigh. I hate that but it's our current standard. There's an issue somewhere...

jhodgdon’s picture

housekeeping with files

dawehner’s picture

On 3 I don't think we do that on getInfo() methods on tests. Sigh. I hate that but it's our current standard. There's an issue somewhere...

Well we certainly do that everywhere now, maybe since #338403: Use {@inheritdoc} on all class methods (including tests) and #1999512: getInfo() should be an abstract method with docs on TestBase/UnitTestCase

jhodgdon’s picture

FileSize
31.33 KB

Oh, I forgot about that actually finally getting fixed! The test code was never patched -- most of them still don't have inheritdoc in them. Anyway here is a patch with the inheritdoc added. I didn't bother with an interdiff -- that is the only change -- in core/modules/views/lib/Drupal/views/Tests/SearchIntegrationTest.php.

dawehner’s picture

Status: Needs review » Reviewed & tested by the community

Thank you. All we need is to write A LOT of new tests, so the usage of {@inheritdoc} wins procentual.

jibran’s picture

One minor thing.

  1. +++ b/core/modules/search/lib/Drupal/search/Plugin/views/argument/Search.php
    @@ -19,25 +22,48 @@
    +  protected $search_query = NULL;
    

    It should be $searchQuery.

  2. +++ b/core/modules/search/lib/Drupal/search/Plugin/views/filter/Search.php
    @@ -7,35 +7,58 @@
    +  protected $search_query = NULL;
    

    Same here $searchQuery.

catch’s picture

Assigned: catch » Unassigned

Don't see any reason to keep the backlinks support, and definitely not at the expense of views support in general. Haven't reviewed the actual patch yet.

jhodgdon’s picture

RE #80 you are correct, but this patch didn't introduce that variable -- it already existed; the line of code was changed to declare it properly. Is it within or out of scope to fix it in this issue?

jhodgdon’s picture

Status: Reviewed & tested by the community » Needs review
FileSize
33.41 KB
7.55 KB

OK, one more patch, fixing the $search_query vs. $searchQuery issue identified in #80, which as I noted above was not introduced by this patch, and which necessitates another round of reviews, but which otherwise will most likely not be cleaned up.

I'm not sure I ever actually uploaded the right patch in #78 either -- it looks like I re-uploaded the #75 patch instead. So you might want to look at the last two interdiffs and the current patch. Sorry about that!

jibran’s picture

Status: Needs review » Reviewed & tested by the community

Thank you for the changes. I think it is RTBC.

catch’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/node/node.views.inc
@@ -627,6 +627,81 @@ function node_views_data() {
+  // Add search table, fields, filters, etc., but only if the a page using the

'the a page'.

Also doesn't this mean I'd not be able to use search to create views filters if the node search page isn't enabled? I'd expect to be able to do that just by having both node + search enabled. However does the new plugin architecture for search mean indexing also doesn't happen? In that case we should just leave as is.

mgifford’s picture

FileSize
24.28 KB

simple re-roll to catch that error to address the issues in the docs.

But ya, @catch's bigger question is pretty critical.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community
FileSize
33.4 KB

Thanks for fixing the docs error mgifford, although the correct fix would be to keep the "a" and not "the", since you can have multiple pages with the same plugins. So, here's a new patch. The interdiff didn't work but I verified using regular diff that the only difference between this and the #83 patch is the "the a" line.

To answer the question in #85, the way the Search plugin system is currently architected, there is no search indexing of nodes unless there is at least one Node Search page defined. Enabling Node and Search will set up a Node Search page, but if it is deleted before the first cron run, your nodes will not be indexed.

This is because the way indexing happens is that during search_cron(), the Search module goes through the list of all enabled Search pages, figures out which ones' plugins have declared they are using the Core Search index (they have implemented an interface to indicate this), and calls their updateIndex() method (from that interface). (For instance, the core User Search plugin doesn't use the index, hasn't implemented the indexing interface, and has no updateIndex() method, while the Node Search plugin does.)

We had essentially the same behavior in Drupal 7, although it didn't use plugins per se. In 7, on the Search Settings page, you could enable/disable search modules (modules that implemented hook_search_info(), each of which defined a search page URL), and hook_update_index() was only called on enabled search modules. So the current patch should behave the same as the Search Views integration did in 7.

And the reason that I think we want to preserve this behavior is for efficiency. If someone is using a contrib module that interacts with the Search system (using its index and/or pages), they may not want core User or Node searches, and in that case, the contrib plugin should be taking care of its indexing needs and you wouldn't want to also waste time indexing nodes in the regular way.

That said, if someone wants to write a contrib module that causes nodes to be indexed in a way identical to the core Node Search plugin, and that also does a hook_views_data() to integrate the indexing data with Views, they could probably do that. Someone who wants to use Views to build their own search page could also do that by enabling the Node Search page (or rather, leaving it enabled) but then not giving people permission to use search.

Is it OK to put this back to RTBC?

The last submitted patch, 86: 1853536-86.patch, failed testing.

mgifford’s picture

@jhodgdon - oops.. Thanks.

catch’s picture

Status: Reviewed & tested by the community » Fixed

Hmm OK. That does seem not much different to 7.x, so let's commit as is. If we wanted to revisit can do so in a follow-up.

Committed/pushed to 8.x, thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.