[META] Porting module to Drupal 8

Yes, but I'd like to get the views integration done first. So that the port may be easier.

Has there been any progress on this?

Nothing happend until now. Do you like to sponsor the port?

I can check with my client; possibly.

It might also be good to put a notice on the project page that you're looking for sponsors for the D8 port, if that's the main issue. That way if we can only partially sponsor it, others could make up the difference.

Is there any update on this project, I would like to help

I want to help, maybe just create a sandbox project and work there ?

I can do some testing if someone's got some rough code available in a sandbox.

We need to get some D7 patches in first as it makes the upgrade a lot easier.

Hi @hass

I am expecting to have some time to work on this in the coming month or so.

I thought I'd propose some high-level architecture for your approval before I start the port as follows:

Step 1 - port the tests. When they go green, we know we're close
Step 2 - create a new entity-type called Link. This will replace the linkchecker_link table and contain all the existing fields. Add a new base field to it by depending on the contributed dynamic_entity_reference module. This field will hold a reference to the entity the link was found in. This will allow referencing any content-entity from the Link entity. This will also replace the linkchecker_block_custom, linkchecker_comment and linkchecker_node tables and allow support for any entity-type, including e.g. paragraphs, field_collection, users etc. This module's functionality is proposed for inclusion in a later version of 8.x - see #2407587: Allow multiple target entity types in the entity reference field. Failing that, we could use the approach comment module takes, storing entity_id and entity_type properties and making each Link bundle only reference one entity-type. Using an entity to store links gives us all the advantages of core's entity APIS - Views integration as standard, action API, REST integration etc. Additionally linkchecker_link_edit_form becomes the entity edit form.
Step 3 - move the admin form to a ConfigForm object and add routing, schema etc
Step 4 - move the batch API callbacks to batch workers objects.
Step 5 - move the per-entity type logic into a new entity handler class 'link checker'. E.g. Workbench moderation adds a new handler to each relevant entity type 'moderation info'. We can add a 'link checker' handler to each entity-type. These would all inherit from a base-class with only the entity-type specific logic in the children. This includes all of the hook_{entity_type}_{hook} and the _linkchecker_(add|delete|cleanup|extract|missing)_{entity_type}_links functions. See workbench_moderation on D8 for how this might look.
Step 7 - move _linkchecker_extract_links to an extraction service and _linkchecker_link_replace to a replacement service
Step 8 - move _linkchecker_parse_fields and _linkchecker_replace_fields to field parsing and replacing services.
Step 9 - Port elements to D8 paradigms, e.g. permissions to yml file, .info to yml file, links to yml files, routing.
Step 10 - port hook_menu access callbacks to AccessCheck objects
Step 11 - port _linkchecker_link_access to an AccessControlHandler for the Link entity
Step 12 - port _linkchecker_check_links to queue processing
Step 13 - Make the queue processor that replaces _linkchecker_check_links fire an event, split _linkchecker_status_handling into event handlers to allow it to be broken up and also other modules to interact
Step 14 - move _linkchecker_cleanup_links to a cleanup service
Step 15 - move linkchecker_admin_report_page and linkchecker_user_report_page to views
Step 16 - move linkchecker.redirect.inc to a service that is only enabled (via a ServiceProviderInterface implementation) if redirect module is enabled
Step 17 - port the drush commands

I'm happy to do all this in a sandbox with atomic commits that could later be merged/squashed or do it via patches here.

Thoughts?

Before we start porting we need to get #2060243: Split linkchecker_scan_nodetypes as content type specific in. Than we can easier migrate to D8.

Step 1 - port the tests. When they go green, we know we're close

Not really. Test coverage is really weak.

Step 2 - create a new entity-type called Link.

Will this not conflict with Link module? I'm not sure what the reasons are for this, but core entity stuff may not fast enough for this module. We need unique keys, hashes and so on to find the links and we cannot load every entity just to search for a link. You need to expect a few million of links. My expierence with core is not so positive if it comes to special requirements. This is not about beeing able to save the data... it is all about performance and flexibility.

Step 4 - move the batch API callbacks to batch workers objects.

Never heard of this... looking forward to your patch.

Step 7 - move _linkchecker_extract_links to an extraction service and _linkchecker_link_replace to a replacement service

Sounds good.

Step 8 - move _linkchecker_parse_fields and _linkchecker_replace_fields to field parsing and replacing services.

Sounds good.

Step 13 - Make the queue processor that replaces _linkchecker_check_links fire an event, split _linkchecker_status_handling into event handlers to allow it to be broken up and also other modules to interact

You mean with actions? That would be useful, I'm just not sure about performance. Keep in mind we are using HTTPRL with parallel link checking and a very high hit rate. We also need to limit requests per server to not killing servers. As I have said HTTPRL I do not think Guzzle has the features of HTTPRL and HTTPRL is not yet available for D8.

Step 14 - move _linkchecker_cleanup_links to a cleanup service

What is the benefit of a service here?

Step 15 - move linkchecker_admin_report_page and linkchecker_user_report_page to views

YES, please. I do not care any longer if we get this into D7, but access checking is very difficult... just as a note.

I'm happy to do all this in a sandbox with atomic commits that could later be merged/squashed or do it via patches here.

I like to make smaller commits. Commits that are reviewable. I made the expierence that the module was broken in several ways after I started to trust one guy. The reason was mainly the patches have been too large to review.

Whatever, this looks all very promising and I'm looking forward to your patches to review. Please let us cleanup the D7 setting stuff upfront (#2060243: Split linkchecker_scan_nodetypes as content type specific).

Ok, will check in over there

As the #2060243: Split linkchecker_scan_nodetypes as content type specific went in, Can we finalize the proposed port plan and create the child issues.
Let me know I would like to help.

Yes. I already started porting. Need your help now.

great hass!
can you create the issues and we'll work on them later soon.

Found the code in the d8 branch.looked at the code and filing couple of issues to get it ported to d8
@hass,
can you create the d8 development snapshot so that we'll choose the version to 8.x-1.x-dev in issues.

Thanks for the great module and your help

we have development snapshot out :) yay!
Changing the issue status from needs review to active as we're not doing any coding here.
(I'm not sure about how the issue statues of the plans change, probably Lee knows better)

looks like help wasn't needed

No, it is still needed. I only made the things I know how to implement, now things are more complicated and require a lot of re-factoring.

oh neat - any particular issues I can look at?

Lee, could you update the #2780873: Add a new entity type linkchecker with your proposed plan including the technical details with DER and we can work on if hass would be happy. I'll try to work on the rest of the issues soon to get's this devil released.

Yeah, that seems to be the most important next step as it is all the module depends on.

I curious at the status of the port and the roadmap for it. Looking at the code I see l there's loads of work to do. Since there is discussion about architectural choices e.g. #2780873: Add a new entity type linkchecker, I'm not sure where to start?

As said in#24, enity type is the next step.

Does this module work at all even in a minimum capacity? if so can I get a document on how this can work. Or how can i help to make this work in a minimal capacity since i have time to work on it now?

#2780873: Add a new entity type linkchecker need to be done next.

@hass I am not sure what needs to be done to add a new entity. Can I get more specific instructions ?

any thoughts as to how to make this work without httprl ? since HTTPRL is not yet available for D8?

@hass it is not better drop the change to entity and just upgrade everything from drupal 7 to 8 using tables as before? The people that proposed this did not any visible work on it for months and everyone else is just waiting. Anyway, we can just replace the classes to use entities latter when it is ready.

@dianacastillo you can generate a new entity type using Drupal console.

The people that proposed this did not any visible work on it for months and everyone else is just waiting

Actually, the people that proposed this are working porting your other modules to Drupal 8. The code is open source, the documentation is too. If this module matters to you, work on patches or sponsor someone who can. Demanding more free work from volunteers and casting aspersions about the amount of work they have/haven't put in doesn't write code.

</rant>

Happy friday morning

The only thing i said is this: people proposed a change that @hass do not know how to make, he hinself said this on other comment, and this same people did not work on it. So simple, make it with @hass know. But i will not discuss about this.

(Good news: I am working on a port to D8. Bad news: I'm not likely to be able to contribute my work to the community.)

I'd still like to give you a heads up about something I just encountered. TL;DR: Multilingual is different in D8. Each translation has the same entity ID.

First off, I decided to do a more or less straight port for my first version. Therefore, I have stuck with the hook_schema approach instead of going the entity route. I've kept all of the same tables.

Here's what I've just run into. I brought the linkchecker_node table across with just nid and lid. Unfortunately, that doesn't cut it. Not only do I need to track nid and lid, I also need to track the language code to know which translation I'm referencing. I don't know if you'll run into the same thing if you go the entity route; I don't know how you're planning to deal with the translation(s) of your entity.

Since I'm still planning to continue with the hook_schema route, here's what I'm planning to do. I'm going to consolidate linkchecker_node, linkchecker_comment and linkchecker_block_custom into a linkchecker_entity table. That table will have the fields: entity_id, entity_type, bundle, langcode and lid.

Here is another multilingual issue you'll need to address.

In addition to dealing with (depending on the route you go)

hook_entity_delete --
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
hook_ENTITY_TYPE_delete --
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

you'll also need to deal with (depending on the route you go)
hook_entity_translation_delete --
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...
hook_ENTITY_TYPE_translation_delete --
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Entity%21...

"I'm not likely to be able to contribute my work to the community", can happen. Whats blocking?

The client is not currently willing to give permission for me to contribute the code.

been there too ;-). some arguments like "if other chip in it could be cheaper/faster" or "community maintenance" can help sometimes.

Porting to Drupal 8.

- Convert code from D7 -> D8.
- Fix errors.
- Basic function works(All node content types)
- Build report page content.

Don't have time to review this myself at the moment but setting status in case someone else can.

I added some more fixing in code.

I fixed hashBase64 bug.

This is not ready for review. Many changes are straight forward, but there are other in modules file that are simply incorrect. e.g. variable get() no longer has a default as I know. In one place $node->type is upgraded with $node->getType() in other place with $node->bundle()? Use of GuzzleHttp cannot removed. There are lots of comments from db connections that could be removed. We need to check if we may need to remove httprl support as this module does not exists yet - or at least comment it. The upgrade of $response->uri['fragment'] looks very unreliable to me if we rely on indexes. A keyed name would be better. Not tested, but an url is not a header. I think it may be more something like $uri = $request->getUri(); per https://github.com/guzzle/guzzle3/blob/master/src/Guzzle/Http/Url.php there is also a getParts() that returns fragment. I also never use target in html links. This is up to the user to open something in a new tab. If fetchObject() is used this cannot be an array ($link['urlhash']). Please remove the lot of added blank lines. 'linkchecker_scan_node_' . $type is not a linkchecker variable.

However there are lot of lines I could take over one by one that seems codewise safe. Maybe the patch could be cleaned up first a little bit.

Adding in a further pass as the patch in #42 was erroring when nodes were saved with nested menu items. This is definitely not a finished patch - rather an incremental fix that should help the progression of this issue.

Found another small issue here relating to status of TRUE/FALSE and MySQL expecting an integer 0 or 1.

Fixing duplicate entry in node_linkchecker insert.

What's the status on committing these up to the git repo? I can understand not putting out a release but seems there are a few patches to improve what's in git.

Patches that contain incorrect changes, cannot committed.

This patch addesses a part of @hass comments at #43 :

• $config->get() without default value
• Removed the extra blank lines
• Removed a few comments from db connections
• Commented all code related to httprl support
• Replaced the 3 $node->getType() occurences with the more generic $node->bundle()

And I added support to Drush9 with drush.services.yml and src/Commands/LinkcheckerCommands.php files.
(As needed for Drupal 8.4.0 / Symfony 3.2)

I guess next steps are :

• improvements
• transform .inc files with services with dependency injection

Thanks for following up. Next step seems to be entity implementation...

1. +++ b/.idea/vcs.xml
   @@ -0,0 +1,6 @@
   +<?xml version="1.0" encoding="UTF-8"?>
   +<project version="4">
   +  <component name="VcsDirectoryMappings">
   +    <mapping directory="$PROJECT_DIR$" vcs="Git" />
   +  </component>
   +</project>
   

   What is this?

2. +++ b/linkchecker.module
   @@ -109,9 +118,10 @@ function linkchecker_help($route_name, RouteMatchInterface $route_match) {
   +    $logger->log($severity, $message, $variables);
   

   Where is $link gone?

3. +++ b/linkchecker.module
   @@ -192,28 +203,32 @@ function _linkchecker_link_node_ids($link, $node_author_account = NULL) {
   + // $linkchecker_scan_nodetypes = linkchecker_scan_node_types();
   + // if (empty($linkchecker_scan_nodetypes) || !\Drupal::currentUser()->hasPermission('access content')) {
   +  //  return array();
   +  //}
   

   Why is this commented?

4. +++ b/linkchecker.module
   @@ -192,28 +203,32 @@ function _linkchecker_link_node_ids($link, $node_author_account = NULL) {
   +
   +    //$query = db_select('node', 'n');
   

   ?

5. +++ b/linkchecker.module
   @@ -192,28 +203,32 @@ function _linkchecker_link_node_ids($link, $node_author_account = NULL) {
   +        ->condition('n.uid', $node_author_account->uid)
   +        ->condition('r.uid', $node_author_account->uid);
   

   Indendation. Just a guess, ->uid is now id(). But we can do this in next step. This way it is easier to read.

6. +++ b/linkchecker.module
   @@ -192,28 +203,32 @@ function _linkchecker_link_node_ids($link, $node_author_account = NULL) {
   +    //$query = db_select('node', 'n');
   

   ?

7. +++ b/linkchecker.module
   @@ -305,7 +320,8 @@ function _linkchecker_link_comment_ids($link, $comment_author_account = NULL) {
   +    //$query = db_select('comment', 'c');
   

   ?

8. +++ b/linkchecker.module
   @@ -314,7 +330,8 @@ function _linkchecker_link_comment_ids($link, $comment_author_account = NULL) {
   +    //$query = db_select('comment', 'c');
   

   ?

9. +++ b/linkchecker.module
   @@ -338,32 +355,37 @@ function _linkchecker_link_comment_ids($link, $comment_author_account = NULL) {
   +  //$bids = db_query('SELECT bid FROM {linkchecker_block_custom} WHERE lid = :lid', array(':lid' => $link->lid))->fetchCol();
   

   ?

10. +++ b/linkchecker.module
    @@ -338,32 +355,37 @@ function _linkchecker_link_comment_ids($link, $comment_author_account = NULL) {
    + // global $user;
    

    ?

11. +++ b/linkchecker.module
    @@ -338,32 +355,37 @@ function _linkchecker_link_comment_ids($link, $comment_author_account = NULL) {
    +  $or_condition_group =  $query->orConditionGroup()
    

    extra blank before $query.

12. +++ b/linkchecker.module
    @@ -338,32 +355,37 @@ function _linkchecker_link_comment_ids($link, $comment_author_account = NULL) {
    +               ->condition('r.rid', $rids, 'IN')
    +               ->isNull('r.rid');
    

    Indendation

13. +++ b/linkchecker.module
    @@ -412,119 +435,129 @@ function _linkchecker_check_links() {
    +      } elseif ($link->method == 'GET') {
    

    Newline before "elseif"

14. +++ b/linkchecker.module
    @@ -412,119 +435,129 @@ function _linkchecker_check_links() {
    +          'headers' => $headers,
    +          'method' => $link->method,
    +          'max_redirects' => 0,
    

    Indendation

15. +++ b/linkchecker.module
    @@ -412,119 +435,129 @@ function _linkchecker_check_links() {
    +            'global_connections' => $linkchecker_check_connections_max,
    +            'global_timeout' => $max_execution_time - 30,
    +            'domain_connections' => $linkchecker_check_domain_connections,
    +            'callback' => [
    +                [
    +                    'function' => '_linkchecker_status_handling',
    +                ],
    +                $link, // This need to be passed or it's not send back to _linkchecker_status_handling()
    +            ],
    

    Indendation

16. +++ b/linkchecker.module
    @@ -412,119 +435,129 @@ function _linkchecker_check_links() {
    +      } else {
    

    Newline before else.

17. +++ b/linkchecker.module
    @@ -412,119 +435,129 @@ function _linkchecker_check_links() {
    +        } catch (\GuzzleHttp\Exception\ClientException $e) {
    

    newline?

18. +++ b/linkchecker.module
    @@ -563,14 +596,16 @@ function _linkchecker_status_handling(&$response, $link) {
    +  $response->code = $response->getStatusCode();
    

    That looks not like clean code.

19. +++ b/linkchecker.module
    @@ -601,42 +636,44 @@ function _linkchecker_status_handling(&$response, $link) {
    +              //Node::create($node);
    

    ??

20. +++ b/linkchecker.module
    @@ -679,7 +718,9 @@ function _linkchecker_status_handling(&$response, $link) {
    +       $connection = \Drupal::database();
    

    Indendation

21. +++ b/linkchecker.module
    @@ -713,25 +754,27 @@ function _linkchecker_status_handling(&$response, $link) {
    +      $response->error = t('Page not found');
    

    That line cannot correct. The error text comes from the remote server.

22. +++ b/linkchecker.module
    @@ -830,9 +880,12 @@ function _linkchecker_status_handling(&$response, $link) {
    +  //variable_del('linkchecker_scan_node_' . $info->type);
    +  //variable_del('linkchecker_scan_comment_' . $info->type);
    +  //Drupal::configFactory()->getEditable('linkchecker_scan_node_' . $info->type)->delete();
    +  //Drupal::configFactory()->getEditable('linkchecker_scan_comment_' . $info->type)->delete();
    

    Add TODO comment, please.

23. +++ b/linkchecker.module
    @@ -840,10 +893,13 @@ function linkchecker_node_type_delete($info) {
    +  $nid =$node->id();
    

    At least a space is missing.

24. +++ b/linkchecker.module
    @@ -840,10 +893,13 @@ function linkchecker_node_type_delete($info) {
    +    //$links = db_query('SELECT ll.* FROM {linkchecker_node} ln INNER JOIN {linkchecker_link} ll ON ln.lid = ll.lid WHERE ln.nid = :nid AND ll.fail_count > :fail_count AND ll.status = :status AND ll.code NOT IN (:codes)', array(':nid' => $node->id(), ':fail_count' => 0, ':status' => 1, ':codes' => $ignore_response_codes));
    

    ?

25. +++ b/linkchecker.module
    @@ -866,14 +922,23 @@ function linkchecker_node_insert($node) {
    +
    +  if (!$node->isDefaultRevision()) {
         return;
       }
    + //if (!_linkchecker_isdefaultrevision($node)) {
    + //   return;
    + // }
     
    +  $node_type = \Drupal\node\Entity\NodeType::load($node->bundle());
    

    Looks like FIXME comment required. Not sure how this need to be changed for content_moderation that goes into D8.5

26. +++ b/linkchecker.module
    @@ -920,8 +990,10 @@ function linkchecker_comment_insert($comment) {
    +  $node_type = $connection->query('SELECT type FROM {node} WHERE nid = :nid', array(':nid' => $comment->nid))->fetchField();
    

    Is this info part of the $comment object in D8?

27. +++ b/linkchecker.module
    @@ -1146,16 +1223,22 @@ function _linkchecker_extract_node_links($node, $return_field_names = FALSE) {
    + // $text_items_by_field['title'][] = _filter_url($node->get('title')->value, $filter);
    

    ?

28. +++ b/linkchecker.module
    @@ -1146,16 +1223,22 @@ function _linkchecker_extract_node_links($node, $return_field_names = FALSE) {
    +  //$languages = language_list();
    

    ?

29. +++ b/linkchecker.module
    @@ -1167,6 +1250,7 @@ function _linkchecker_extract_node_links($node, $return_field_names = FALSE) {
    +
    
    @@ -1191,7 +1275,9 @@ function _linkchecker_extract_node_links($node, $return_field_names = FALSE) {
    +
    ...
    +
    
    @@ -1204,32 +1290,49 @@ function _linkchecker_extract_node_links($node, $return_field_names = FALSE) {
    +
    

    unrelated changes

30. +++ b/linkchecker.module
    @@ -1204,32 +1290,49 @@ function _linkchecker_extract_node_links($node, $return_field_names = FALSE) {
    +        $lid = \Drupal::database()->insert('linkchecker_link')
    +           ->fields($link)
    +           ->execute();
    +
    +        \Drupal::database()->insert('linkchecker_node')
    +            ->fields([
    +                'nid' => $node->id(),
    +                'lid' => $lid,
    +            ])
    

    Indendation

31. +++ b/linkchecker.module
    @@ -1204,32 +1290,49 @@ function _linkchecker_extract_node_links($node, $return_field_names = FALSE) {
    +              ->fields([
    +                  'nid' => $node->id(),
    +                  'lid' => $link->lid,
    +              ])
    +              ->execute();
    

    Indendation

32. +++ b/linkchecker.module
    @@ -1254,20 +1357,21 @@ function _linkchecker_add_node_links($node, $skip_missing_links_detection = FALS
    + _linkchecker_cleanup_node_references($node->id(), $links);
    

    indendation

33. +++ b/linkchecker.module
    @@ -1307,20 +1411,26 @@ function _linkchecker_add_comment_links($comment, $skip_missing_links_detection
    +        //drupal_write_record('linkchecker_link', $link);
    

    ?

34. +++ b/linkchecker.module
    @@ -1307,20 +1411,26 @@ function _linkchecker_add_comment_links($comment, $skip_missing_links_detection
    +
    

    ?

35. +++ b/linkchecker.module
    @@ -1411,20 +1521,25 @@ function _linkchecker_add_block_custom_links($block_custom, $bid, $skip_missing_
    +        //drupal_write_record('linkchecker_link', $link);
    

    ?

36. +++ b/linkchecker.module
    @@ -1571,7 +1687,8 @@ function _linkchecker_cleanup_comment_references($cid = 0, $links = array()) {
    +
    

    remove blank line

37. +++ b/linkchecker.module
    @@ -1579,11 +1696,12 @@ function _linkchecker_cleanup_block_custom_references($bid = 0, $links = array()
    +
    

    remove blank line

38. +++ b/linkchecker.module
    @@ -1602,7 +1720,11 @@ function _linkchecker_cleanup_block_custom_references($bid = 0, $links = array()
    +
    +  $connection = \Drupal::database();
    +
    +  $result = $connection->query('SELECT ll.url FROM {linkchecker_link} ll INNER JOIN {linkchecker_node} ln ON ln.lid = ll.lid WHERE ln.nid = :nid AND ll.urlhash IN (:urlhashes[])', [':nid' => $nid, ':urlhashes[]' => array_map('\Drupal\Component\Utility\Crypt::hashBase64', $links)]);
    +
    

    remove blank lines

39. +++ b/linkchecker.module
    @@ -1622,7 +1744,10 @@ function _linkchecker_node_links_missing($nid, $links) {
    +   $connection = \Drupal::database();
    

    indendation

40. +++ b/linkchecker.module
    @@ -1672,6 +1799,7 @@ function _linkchecker_block_custom_links_missing($bid, $links) {
    +
    

    remove blank line

41. +++ b/linkchecker.module
    @@ -1680,55 +1808,67 @@ function _linkchecker_parse_fields($entity_type, $bundle_name, $entity, $return_
    +         // foreach ($entity_field as $value) {
    

    indendation

42. +++ b/linkchecker.module
    @@ -1680,55 +1808,67 @@ function _linkchecker_parse_fields($entity_type, $bundle_name, $entity, $return_
    +                'format' => NULL,
    +                'summary' => '',
    +                'value' => '',
    

    indendation

43. +++ b/linkchecker.module
    @@ -1680,55 +1808,67 @@ function _linkchecker_parse_fields($entity_type, $bundle_name, $entity, $return_
    +         $field_value =  $entity_field->getValue();
    

    indendation

44. +++ b/linkchecker.module
    @@ -1680,55 +1808,67 @@ function _linkchecker_parse_fields($entity_type, $bundle_name, $entity, $return_
    +                  'title' => '',
    

    indendation

45. +++ b/linkchecker.module
    @@ -1802,11 +1942,13 @@ function _linkchecker_cleanup_links() {
    +
    ...
    +
    

    remove blank lines

46. +++ b/linkchecker.module
    @@ -1815,7 +1957,8 @@ function _linkchecker_cleanup_links() {
    +    //db_truncate('linkchecker_node')->execute();
    +     \Drupal::database()->truncate('linkchecker_node')->execute();
    

    ? and indendation

47. +++ b/linkchecker.module
    @@ -1823,31 +1966,35 @@ function _linkchecker_cleanup_links() {
    +
    ...
    +
    

    remove blank lines

48. +++ b/linkchecker.module
    @@ -1823,31 +1966,35 @@ function _linkchecker_cleanup_links() {
    +
    ...
    +
    

    remove blank lines

49. +++ b/linkchecker.module
    @@ -1823,31 +1966,35 @@ function _linkchecker_cleanup_links() {
    +  $subquery2  = \Drupal::database()->select($linkchecker_block_custom->union($linkchecker_comment)->union($linkchecker_node), 'q1')
    

    remove double space

50. +++ b/linkchecker.module
    @@ -1871,12 +2018,12 @@ function _linkchecker_cleanup_links() {
    +
    

    remove blank line

51. +++ b/linkchecker.module
    @@ -2081,17 +2227,17 @@ function _linkchecker_link_replace(&$text, $old_link_fqdn = '', $new_link_fqdn =
    +        Unicode::strtolower('http://' . $_SERVER['HTTP_HOST']),
    +        Unicode::strtolower('https://' . $_SERVER['HTTP_HOST']),
    

    indendation

52. +++ b/linkchecker.module
    @@ -2307,29 +2455,40 @@ function _linkchecker_check_markup($text, $format_id = NULL, $langcode = '', $ca
    +  //$filters = filter_list_format($format->format);
    +  //$filter_info = filter_get_filters();
    

    ?

53. +++ b/linkchecker.module
    @@ -2307,29 +2455,40 @@ function _linkchecker_check_markup($text, $format_id = NULL, $langcode = '', $ca
    +
    ...
    +
    ...
    +
    ...
    +
    

    remove blank lines

54. +++ b/linkchecker.module
    @@ -2307,29 +2455,40 @@ function _linkchecker_check_markup($text, $format_id = NULL, $langcode = '', $ca
    +
    

    remove blank line

55. +++ b/linkchecker.module
    @@ -2393,17 +2552,17 @@ function _linkchecker_absolute_content_path($url) {
    +  $status = 1;
    ...
    +    $status = 0;
    

    Why is this changed to integer?

56. +++ b/linkchecker.module
    @@ -2527,7 +2688,9 @@ function _linkchecker_unpublish_nodes($lid) {
    +
    

    remove blank line

57. +++ b/linkchecker.module
    @@ -2558,13 +2721,13 @@ function _linkchecker_isdefaultrevision($entity) {
    +   // if (module_exists('workbench_moderation') && workbench_moderation_node_type_moderated($entity->type) === TRUE && empty($entity->workbench_moderation['updating_live_revision'])) {
    +   // if (\Drupal::moduleHandler()->moduleExists('workbench_moderation') && ($entity->hasHandlerClass('moderation')) && empty($entity->workbench_moderation['updating_live_revision'])) {
    +  //   return FALSE;
    +  // }
     
    -  return TRUE;
    +  //return TRUE;
    

    May needs a todo/fixme comment

58. +++ b/linkchecker.redirect.inc
    @@ -17,14 +17,13 @@ function linkchecker_redirect_insert($redirect) {
    +  $url_http = Url::fromUri('internal:' . $redirect->source);
    ...
    +    $url_http->toString(),
    +    $url_https->toString(),
    +    rawurldecode($url_http->toString()),
    +    rawurldecode($url_https->toString()),
    

    Let's move this ->toString() to Url::fromUri()->toString()

59. +++ b/linkchecker.routing.yml
    @@ -34,4 +34,4 @@ linkchecker.edit_link_settings_form:
    \ No newline at end of file
    

    Add newline

60. +++ b/src/Form/LinkCheckerAdminSettingsForm.php
    @@ -310,7 +312,8 @@ class LinkCheckerAdminSettingsForm extends ConfigFormBase {
    +
    

    remove empty line

61. +++ b/src/Form/LinkCheckerAdminSettingsForm.php
    @@ -380,10 +383,11 @@ class LinkCheckerAdminSettingsForm extends ConfigFormBase {
    +
    

    remove empty line

62. +++ b/src/Form/LinkCheckerEditLinkSettingsForm.php
    @@ -67,34 +67,37 @@ class LinkCheckerEditLinkSettingsForm {
    +      //db_update('linkchecker_link')
    

    ?

63. +++ b/src/Form/LinkCheckerEditLinkSettingsForm.php
    @@ -67,34 +67,37 @@ class LinkCheckerEditLinkSettingsForm {
    +     // db_update('linkchecker_link')
    

    ?

64. +++ b/src/Form/LinkCheckerEditLinkSettingsForm.php
    @@ -67,34 +67,37 @@ class LinkCheckerEditLinkSettingsForm {
    +      //db_update('linkchecker_link')
    

    ?

I had to disable this module because I ran into this problem while editing text:

Error: Call to undefined function variable_get() in linkchecker_scan_node_types() (line 2478 of /modules/linkchecker/linkchecker.module)

Thanks @hass for this thorough review & the response to my post in #47.

@pguillard you able to put out another patch?

@mgifford I'm on it, but this needs time

Here is an improvement according to most of comments at #51

• Removed comment lines that remained after the D8 port
• Corrected PHPCS things
• Added @todos
• Replaced $comment->nid with $comment->getCommentedEntityId(), not tested
• Removed $response->error = t('Page not found');
• linkchecker_node_prepare() replaced with linkchecker_node_prepare_form()
• Replaced if (arg(0) == 'node' && is_numeric(arg(1)) && arg(2) == 'edit' && isset($nid)) with if (!$node->isNew())

There is still a lot of work to be done !

I added :

• some more PHPCS corrections, empty lines etc..
• passing most array() => [] conversions
• Some $account->uid => $account->id() conversions
• Corrected DB query in _linkchecker_report_page()

Thanks for the cleanup. I reviewed it code wise and will commit this first step soon with some changes. It will require some more smaller iterations to get to a working module, but this is a great step forward.

Please try to post smaller patches that can be easier and faster reviewed. May split up in sub cases, please.

1. +++ b/linkchecker.module
   @@ -94,24 +109,25 @@ function linkchecker_help($route_name, RouteMatchInterface $route_match) {
   +function linkchecker_watchdog_log($type, $message, $variables = [], $severity = RfcLogLevel::NOTICE, $link = NULL) {
   +  if ($severity <= \Drupal::config('linkchecker.settings')->get('logging.level')) {
   +    $logger = \Drupal::logger($type);
   +    $logger->log($severity, $message, $variables);
   

   Still unclear to me what happened with $link. It's not saved to log.

2. +++ b/linkchecker.module
   @@ -338,33 +355,35 @@ function _linkchecker_link_comment_ids($link, $comment_author_account = NULL) {
      // Otherwise, only return blocks that this user (or anonymous users) have
      // access to.
   -  global $user;
   -  $rids = array_keys($user->roles);
   -  $rids[] = DRUPAL_ANONYMOUS_RID;
   +  $rids = array_keys($user->getRoles());
   +  $rids[] = AccountInterface::ANONYMOUS_ROLE;
   

   Does this work? $user may not exists here after the patch.

3. +++ b/linkchecker.module
   @@ -830,23 +897,28 @@ function _linkchecker_status_handling(&$response, $link) {
      // Node edit tab is viewed.
   -  if (arg(0) == 'node' && is_numeric(arg(1)) && arg(2) == 'edit' && isset($node->nid)) {
   +  if (!$node->isNew()) {
   

   That looks code wise not correct to me. What has this to do with view edit tab?

4. +++ b/linkchecker.module
   @@ -920,8 +1007,10 @@ function linkchecker_comment_insert($comment) {
   +
   +  if (\Drupal::config('linkchecker.settings')->get('linkchecker_scan_comment_' . $node_type) && $comment->status == COMMENT_PUBLISHED) {
        _linkchecker_add_comment_links($comment);
      }
      else {
   

   I guess this needs work, too. Comment->status may no longer exists with the constant.

5. +++ b/linkchecker.module
   @@ -1204,32 +1293,49 @@ function _linkchecker_extract_node_links($node, $return_field_names = FALSE) {
   -        $link = new stdClass();
   -        $link->urlhash = $urlhash;
   -        $link->url = $url;
   -        $link->status = _linkchecker_link_check_status_filter($url);
   -        drupal_write_record('linkchecker_link', $link);
   +        $link = [];
   +        $link['urlhash'] = $urlhash;
   +        $link['url'] = $url;
   +        $link['status'] = _linkchecker_link_check_status_filter($url);
   

   Any reason to change this to an array?

6. +++ b/linkchecker.module
   @@ -1204,32 +1293,49 @@ function _linkchecker_extract_node_links($node, $return_field_names = FALSE) {
   -        $link = new stdClass();
   -        $link->urlhash = $urlhash;
   -        $link->url = $url;
   -        $link->status = _linkchecker_link_check_status_filter($url);
   -        drupal_write_record('linkchecker_link', $link);
   +        $link = [];
   +        $link['urlhash'] = $urlhash;
   +        $link['url'] = $url;
   +        $link['status'] = _linkchecker_link_check_status_filter($url);
   +
   +        $lid = \Drupal::database()->insert('linkchecker_link')
   +          ->fields($link)
   +          ->execute();
   +
   +        \Drupal::database()->insert('linkchecker_node')
   +          ->fields([
   +            'nid' => $node->id(),
   +            'lid' => $lid,
   +          ])
   +          ->execute();
   +      }
   +      else {
   +        $result = \Drupal::database()->query("SELECT * FROM {linkchecker_node} WHERE nid = :nid AND lid = :lid", [':nid' => $node->id(), ':lid' => $link->lid])->fetchAll();
   +        if (!$result) {
   +          \Drupal::database()->insert('linkchecker_node')
   +            ->fields([
   +              'nid' => $node->id(),
   +              'lid' => $link->lid,
   +            ])
   +            ->execute();
   +        }
          }
   -      db_insert('linkchecker_node')
   -        ->fields(array(
   -          'nid' => $node->nid,
   -          'lid' => $link->lid,
   -        ))
   -        ->execute();
   

   Need to review this closer later.

7. +++ b/linkchecker.module
   @@ -1579,11 +1691,12 @@ function _linkchecker_cleanup_block_custom_references($bid = 0, $links = array()
   -    db_delete('linkchecker_block_custom')
   +    $subquery = \Drupal::database()->select('linkchecker_link')
   +      ->fields('linkchecker_link', ['lid'])
   +      ->condition('urlhash', array_map('\Drupal\Component\Utility\Crypt::hashBase64', $links), 'IN');
   +
   +    \Drupal::database()->delete('linkchecker_block_custom')
   

   Review if we may use faster truncate over delete.

8. +++ b/linkchecker.module
   @@ -1672,63 +1790,72 @@ function _linkchecker_block_custom_links_missing($bid, $links) {
   +        // Link module field, http://drupal.org/project/link.
   +        case 'link_field':
   +          foreach ($entity_field as $language) {
   +            foreach ($language as $item) {
   +              $item += [
   +                'title' => '',
   +              ];
   +              $options = drupal_parse_url(link_cleanup_url($item['url']));
   +              $text_items[] = $text_items_by_field[$field['field_name']][] = l($item['title'], $options['path'], $options);
   +              $text_items[] = $text_items_by_field[$field['field_name']][] = _linkchecker_check_markup($item['title'], NULL, linkchecker_entity_language($entity_type, $entity), TRUE);
   +            }
   

   This is now in core. Need verificiation if all code is still the same. At least it required documentation changes in the code.

9. +++ b/linkchecker.module
   @@ -1871,12 +2001,12 @@ function _linkchecker_cleanup_links() {
   +  if (\Drupal::config('linkchecker.settings')->get('extract.from_a') == 1) {
        $links = $html_dom->getElementsByTagName('a');
   +
        foreach ($links as $link) {
   

   Remove empty line.

10. +++ b/linkchecker.module
    @@ -2393,17 +2529,16 @@ function _linkchecker_absolute_content_path($url) {
    -  $status = TRUE;
    -
    +  $status = 1;
       // Is url in domain blacklist?
    -  $urls = variable_get('linkchecker_disable_link_check_for_urls', LINKCHECKER_RESERVED_DOCUMENTATION_DOMAINS);
    +  $urls = \Drupal::config('linkchecker.settings')->get('check.disable_link_check_for_urls');
       if (!empty($urls) && preg_match('/' . implode('|', array_map(create_function('$links', 'return preg_quote($links, \'/\');'), preg_split('/(\r\n?|\n)/', $urls))) . '/', $url)) {
    -    $status = FALSE;
    +    $status = 0;
       }
     
       // Protocol whitelist check (without curl, only http/https is supported).
       if (!preg_match('/^(https?):\/\//i', $url)) {
    -    $status = FALSE;
    +    $status = 1;
    

    Rollback to boolean. No idea why this was changed.

Cool, thanks @hass too for your hints. I'll work on that

Some more cleanup, from #56 :

• linkchecker-porting-to-d8-2422035-58.PART2.patch is a patch with these last updates only (I didn't found out how to split the things)
• linkchecker-porting-to-d8-2422035-58.patch is a "normal/usual" patch with its interdiff

1. It seems $variable['%link'] is set in $variable before the function call (I didn't test all cases)
2. This was added at the beginning of the function

function _linkchecker_link_block_ids($link) {
+  $user = Drupal::currentUser();

3. arg(0) beeing D7, the new way I guess is to detect that the node already exists, that means we are in edit mode. At least this is my way, I tested that it works.
4. Right $comment->status is D7, replaced $comment->status => $comment->getStatus()
5. I guess this is the reason somebody did that before :
TypeError: Argument 1 passed to Drupal\Core\Database\Query\Insert::fields() must be of the type array, object given, called in /home/dev/projet/web/modules/contrib/linkchecker/linkchecker.module on line 1322 in Drupal\Core\Database\Query\Insert->fields() (line 70 of core/lib/Drupal/Core/Database/Query/InsertTrait.php).
6. Did not work on it
7. Added a todo // @todo: Review if we may use faster truncate over delete.
8. Right, I changed that part that was D7
9. Empty line removed
10. Rollback to boolean done. There was also a FALSE translated to a 1 (Not tested)

And I found some others (managed in this patch) :

• $form_state['values'] => $form_state->getValues()
• arg(0) => \Drupal::service('path.current')->getPath()[1];

#58 I reviewed this monster patch and committed the attached one. I changed some things, fixed leftovers and reverted the incorrect drupal_write_record() migration. Please see https://www.drupal.org/node/2340291 how drupal_write_record() will be upgraded (merge query). This merge query looks a lot simpler than the proposed code change.

The smaller followup patch from #58 needs a re-role now.

I'm attaching a patch with work I've started on cleaning up the linkchecker.batch.inc file.

Still TODO: Handle blocks now that they've been split into config and entity blocks.

Given that code was committed in #59 iI'd recommend that this issue should be closed/fixed and new issues opened against 8.x branch. If you want this to be a "META" issue for the D8 status then just create related issue links. The intent of D8 module migration was never to try and handle the entire module port in a single giant issue.

I agree with @mpotter
I will reroll my work in #58 next sunday, back at home.

@merauluka:

1. +++ b/linkchecker.batch.inc
   @@ -10,7 +13,7 @@
   +  $result = \Drupal::query('SELECT n.nid FROM {node_field_data} n WHERE n.status = :status AND n.type IN (:types) ORDER BY n.nid', [':status' => 1, ':types' => $node_types]);
   
   @@ -118,11 +121,11 @@ function _linkchecker_batch_comments_import_finished($success, $results, array $
   +  $result = \Drupal::query('SELECT id FROM {block_content_field_data} ORDER BY id');
   

   I have some objections on this patch. I have never heard "\Drupal::query() and I cannot find any documentation about this supported. Not tested.

   Arrays need to be ":types[]" in "IN" statements as I know.

2. +++ b/linkchecker.batch.inc
   @@ -63,8 +66,8 @@ function _linkchecker_batch_node_import_finished($success, $results, array $oper
   +  $result = \Drupal::query('SELECT c.cid FROM {comment_field_data} c INNER JOIN {node_field_data} n ON c.entity_id = n.nid WHERE c.status = :cstatus AND n.status = :nstatus AND n.type IN (:types) ORDER BY c.cid', [':cstatus' => COMMENT_PUBLISHED, ':nstatus' => 1, ':types' => $node_types]);
   

   COMMENT_PUBLISHED needs migration, see https://www.drupal.org/node/2156155.

   There is also the same array / IN statement bug

I think we should keep this case open as meta as otherwise someone will open another one for this :-).

Let's go for individual sub cases as already suggested.

@hass So should I move my work (and updates after your review) into a new ticket for the install file or leave them and continue work for it here?

Please create new tickets.

hi is there a ticket related to the progress and task left in the port so that we can track the status and do the left over of issues

As noted earlier the #2780873: Add a new entity type linkchecker need to be the next step as it affects nearly everything.

hi hass,

Just a query: for having a basic functioning of the module is it necessary to have the entity linkchecker , or first we can have a basic functionality covered in alpha release and then go on with this implementation for next release?

Fixing Error: Call to undefined function drupal_write_record() in _linkchecker_add_node_links() (line 1317 of /var/www/mw/docroot/modules/contrib/linkchecker/linkchecker.module) #0 /var/www/mw/docroot/modules/contrib/linkchecker/linkchecker.module(973): _linkchecker_add_node_links(Object(Drupal\node\Entity\Node)) #1 [internal function]: linkchecker_node_update(Object(Drupal\node\Entity\Node)) #2 /var/www/mw/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(402): call_user_func_array('linkchecker_nod...', Array) #3 /var/www/mw/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(167): Drupal\Core\Extension\ModuleHandler->invokeAll('node_update', Array) #4 /var/www/mw/docroot/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(730): Drupal\Core\Entity\EntityStorageBase->invokeHook('update', Object(Drupal\node\Entity\Node)) #5

Fixing Error: Call to undefined function drupal_write_record() in _linkchecker_add_node_links() (line 1317 of /var/www/mw/docroot/modules/contrib/linkchecker/linkchecker.module) #0 /var/www/mw/docroot/modules/contrib/linkchecker/linkchecker.module(973): _linkchecker_add_node_links(Object(Drupal\node\Entity\Node)) #1 [internal function]: linkchecker_node_update(Object(Drupal\node\Entity\Node)) #2 /var/www/mw/docroot/core/lib/Drupal/Core/Extension/ModuleHandler.php(402): call_user_func_array('linkchecker_nod...', Array) #3 /var/www/mw/docroot/core/lib/Drupal/Core/Entity/EntityStorageBase.php(167): Drupal\Core\Extension\ModuleHandler->invokeAll('node_update', Array) #4 /var/www/mw/docroot/core/lib/Drupal/Core/Entity/ContentEntityStorageBase.php(730): Drupal\Core\Entity\EntityStorageBase->invokeHook('update', Object(Drupal\node\Entity\Node)) #5

Current error: Uncaught PHP Exception Symfony\Component\Routing\Exception\RouteNotFoundException: "Route "dblog.overview" does not exist." at /mnt/www/html/si/docroot/core/lib/Drupal/Core/Routing/RouteProvider.php line 201 request_id="v-e98fe8b2-c734-11e8-b5b1-2f95e838bff8"

Happens when going to admin page.

@kellyimagined, I'm not getting this error with a fresh install. I assume it's on the admin/config/content/linkchecker page? The only field that uses this would be "Update permanently moved links". Do you have any specific settings? And do you have the dblog module enabled? Your issue might be related to issue #2572285.

I think the most work is done. I will take a look at the related issues in this issue. But there a new one for preparing a first alpha release: #3107765: Create first D8 alpha release

I guess we can close this issue now that the modules has an alpha release.