Hello,

This task is intended to store the ideas of improvement of the module. When you see anything you think it will improve the module (documentation needed, coding style, refactoring, ...) post a comment or edit the issue body.

  • Using the Drupal API to avoid database requests using MySQL
  • The functions protected_node_enter_any_password (protected_node.fork.inc) and protected_node_enterpassword (protected_node.redirect.inc) have a lot of code in common (which also should be cleaned)

Thanks.

CommentFileSizeAuthor
#18 using_database_api-2146795-18.patch20.67 KBGrimreaper
#15 using_database_api-2146795-15.patch20.63 KBGrimreaper
#13 using_database_api-2146795-13.patch15.75 KBGrimreaper
#10 using_database_api-2146795-10.patch16.39 KBGrimreaper
#7 using_database_api-2146795-7.patch16.68 KBGrimreaper
#4 using_database_api-2146795-4.patch19.68 KBGrimreaper
#3 using_database_api-2146795-3.patch17.43 KBGrimreaper
#2 using_database_api-2146795-2.patch5.6 KBGrimreaper
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented

Implement the automated tests ?

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
Assigned: Unassigned » Grimreaper
FileSize
using_database_api-2146795-2.patch5.6 KB

Hello,

Here is a patch which rewrite the sql queries to use the database API.

I don't have rewrite a query which in the protected_node.install file because it is commented.

Warning : I don't have tested the patch yet (no time), I have just written it

Thanks for the test and the review.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
FileSize
using_database_api-2146795-3.patch17.43 KB

I forgot a lot of db_query as there was not a @todo tag near them.

WARNING : the patch is not operational. It's introduce errors.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
FileSize
using_database_api-2146795-4.patch19.68 KB

Should be tested, I have no more crash on basic usage and the admin page works.

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
Status: Active » Needs review
cashwilliams’s picture

cashwilliams CreditAttribution: cashwilliams commented
Status: Needs review » Needs work

patch needs a re-roll

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
Status: Needs work » Needs review
FileSize
using_database_api-2146795-7.patch16.68 KB

Effectively, thanks for pointing it.

Should be usable now.

izus’s picture

izus CreditAttribution: izus commented

Hi,
Thanks for providing the apatch
i would prefer some input from other users but we can't always get what we want !
i tried reviewing the patch and here is the result.

  1. +++ b/protected_node.module
@@ -987,26 +1014,27 @@
+// function protected_node_db_rewrite_sql($query, $primary_table, $primary_field, $args) {

    actually this hook is a Drupal 6 one, it should be replaced by https://api.drupal.org/api/drupal/modules!system!system.api.php/function... from Drupal 7 

  2. +++ b/protected_node.module
@@ -1069,12 +1097,17 @@
+      ->field('protected_nodes', array('nid'))

    request will not be successful, the methode name you want here is fields (with s) 

  3. +++ b/protected_node.module
@@ -1161,11 +1204,17 @@
+  ->field('protected_nodes', array('protected_node_is_protected'))

    request will not work. the method name is fields not field 

  4. +++ b/protected_node.settings.inc
@@ -752,16 +784,20 @@
-  $sql .= " WHERE pn.nid IS NULL)";

    This doesn't seem to work.
    you may want to combien a db_select and db_insert as in https://drupal.org/node/310079
    or maybe use a db_query

izus’s picture

izus CreditAttribution: izus commented
Status: Needs review » Needs work
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
Status: Needs work » Needs review
FileSize
using_database_api-2146795-10.patch16.39 KB

Thanks for the review.

1. In my patch I comment it, in this patch (10) I simply delete it. So ? I need to implements hook_query_alter ?

2 and 3 : OK, I found a third place where I miss the "s".

4. OK.... I tried your proposition, but it seems strange. I don't test it.

izus’s picture

izus CreditAttribution: izus commented

yes we need a hook_query_alter to do the same logic as drupal6 version.

for 4, you can try the two versions sql resquest via devel/php for exemple. we can in addition check if $query doesn't return anything and how it will behave with db_select

Thanks

izus’s picture

izus CreditAttribution: izus commented
Status: Needs review » Needs work
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
Status: Needs work » Needs review
FileSize
using_database_api-2146795-13.patch15.75 KB

I tried to implements the hook_query_alter, I don't see how to reproduce the d6 logic, and I think we don't need it.

Protected_node_db_rewrite_sql is no more executed in D7 and I think we would have more bugs reported if it was needed.

So in the last patch, I keep it in the same state as the last code rebase, and I think we will see it later. I don't want the whole patch to be blocked for this hook.

4. I decompose in small steps because I had errors all the time, even with the previous implementation. I know that is not optimized, and I will be better in using the batch API. But it works.

Thanks for the review.

PS : and the previous patch needed to be rebased. :)

izus’s picture

izus CreditAttribution: izus commented

oki let's do the following :
1) create an issue where we focus on the hook_query alter thing to digg more wheather we need it or not
2) rebase this last path as it seems good but needs a rebase
3)for the sake of progress we merge it and keep progressing :)

Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
FileSize
using_database_api-2146795-15.patch20.63 KB

1) done
2) done
3) ok, in the future, I think I will prefer making shorter patches :)
4) warning : in protected_node.module, there were two db request wrong, one in protected_node_and_attachment and the other one in hook_file_download. Two request with a lot of joins, now it should be good. About the one in protected_node_and_attachment, it took me time to realise the tables no more exists in D7, as you made in your patch on the bug on private files.

Now it should be good, but I prefer to warn, if you want to check another time.

5) searching for 4), I discovered how to use count directly in the db request, used in the admin form for stats.

Thanks for the review.

izus’s picture

izus CreditAttribution: izus commented 
+++ b/protected_node.module
@@ -334,61 +334,67 @@
+  $query->join('file_usage', 'fu', 'n.nid = fu.id');

fu.id can target something else than a node, it can be any entity type, we should filter of node in the 'type' colum of file_usage table

izus’s picture

izus CreditAttribution: izus commented
Status: Needs review » Needs work
Grimreaper’s picture

Grimreaper
Primary language French
Location France 🇫🇷
CreditAttribution: Grimreaper commented
Status: Needs work » Needs review
FileSize
using_database_api-2146795-18.patch20.67 KB

$query->condition('fu.type', 'node'); added

izus’s picture

izus CreditAttribution: izus commented
Status: Needs review » Fixed

merged
hope this has no bugs in it, i reviewed it each time but guess this is not sufficient for such a change, we lack testing-focused people.
Thanks !

Status: Fixed » Closed (fixed)

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