The conversion of weblinks_embed from D6 to D7 did not cater for the changes to hook_filter_info and the associated callback functions. In particular, the defaults are stored in hook_filter_info() and the applied settings are stored in db table 'filter'. In D6 these were all done in external variables. For more details see /function/hook_filter_info/7 and /function/callback_filter_settings/7

The problems can be seen if you edit a filter setting, for example 'make groups collapsible'. If changed to 'No' then when you come back next time the value is 'yes'. The changes are not too difficult and I've started working on them.

CommentFileSizeAuthor
#20 2564473_20.weblinks_embed_automated_tests_and_code_fix.patch24.75 KBjonathan1055
#18 2564473_18.weblinks_embed_automated_tests_only.patch11.61 KBjonathan1055
#13 2564473_13.weblinks_embed_automated_tests_only.patch11.71 KBjonathan1055
#9 2564473_9.weblinks_embed_automated_tests.patch5.13 KBjonathan1055
#6 2564473_6.weblinks_embed_automated_tests.patch5.17 KBjonathan1055
#4 2564473_4.weblinks_embed_automated_tests.patch5.2 KBjonathan1055

Comments

jonathan1055 created an issue. See original summary.

jonathan1055’s picture

jonathan1055 commented
Issue summary: View changes
gstegemann’s picture

gstegemann commented

Thanks for discovering this. I really didn't know about the changes in hook_filter_info.

jonathan1055’s picture

jonathan1055 commented
Status: Active » Needs review
StatusFileSize
new 2564473_4.weblinks_embed_automated_tests.patch5.2 KB

I'd like to get some automated tests running for the Embed sub-module. The attached patch simply adds the .test file and runs some intial set-up in the testcase, it does not actually check anything of substance in the module yet. It's better to check that tests are running first, then I will commit this, and then work on the actual test cases. I seem to recall that the new tests do not actually run when the patch to be tested includes the .info change which adds the .test file, but we'll see.

Status: Needs review » Needs work

The last submitted patch, 4: 2564473_4.weblinks_embed_automated_tests.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Related issues: +#460622: Additional automated tests
StatusFileSize
new 2564473_6.weblinks_embed_automated_tests.patch5.17 KB

I forgot that in D.O. automated testing some of the PHP functionality differs from my local environment.

// This works for my automated tests but not on D.O.
$term = taxonomy_get_term_by_name($settings['name']);
$group_tid = array_keys($term)[0];

// This is the longer way to do it, for D.O.
$terms = taxonomy_get_term_by_name($settings['name']);
$term = reset($terms);
$group_tid = $term->tid;
jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review

Has to be 'needs review' to get the test run

Status: Needs review » Needs work

The last submitted patch, 6: 2564473_6.weblinks_embed_automated_tests.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Needs review
StatusFileSize
new 2564473_9.weblinks_embed_automated_tests.patch5.13 KB

I left in a dd() call.

gstegemann’s picture

gstegemann commented
Status: Needs review » Reviewed & tested by the community

I tested your patch and ran a local test which passed.

However, the patch could not be applied to file 'weblinks_embed.info' and a reject file was created: 

--- contribs/weblinks_embed/weblinks_embed.info
+++ contribs/weblinks_embed/weblinks_embed.info
@@ -1,5 +1,7 @@
 name = Web Links Embed
 description = Allows users to include a link group in a node with [links-embed: id].
 core = 7.x
+files[] = weblinks_embed.test
 dependencies[] = weblinks
 package = Weblinks
+configure = admin/config/content/formats

So I had to apply the changes manually.

jonathan1055’s picture

jonathan1055 commented

Yes, patching the .info file can fail if you are using a dev release (which has the datestamp and release information added in the file) and the patch was created in a git environment where the date and release details do not exist in the file.

The .test file needs much more work, but this is a good start to get the tests running. Thanks.

  • jonathan1055 committed ff03881 on 7.x-1.x 
    Issue #2564473 by jonathan1055: Added Weblinks-embed.test file
jonathan1055’s picture

jonathan1055 commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2564473_13.weblinks_embed_automated_tests_only.patch11.71 KB

I've worked on the embed code and the tests. Here is a patch with just the updated tests - and this should fail in several places, showing that the embed functionality is not currently working. Later, I will upload a patch which has the tests and the corrected code.

Status: Needs review » Needs work

The last submitted patch, 13: 2564473_13.weblinks_embed_automated_tests_only.patch, failed testing.

gstegemann’s picture

gstegemann commented
Status: Needs work » Reviewed & tested by the community

I've applied the patch and a test run produced similar results as the run on d.o. did.

  • jonathan1055 committed f66f57e on 7.x-1.x 
    Issue #2564473 by jonathan1055: Weblinks-embed updated tests
jonathan1055’s picture

jonathan1055 commented

Thanks. I'm going to add another loop to show an empty group.
Also I've realised that the unpublished group fails badly for users who do not have the authority to view unpublished content. I may add a test for that too, but I've committed what you have checked so far, so that it is easy to see the subsequent new bits.

jonathan1055’s picture

jonathan1055 commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2564473_18.weblinks_embed_automated_tests_only.patch11.61 KB

As promised here is patch with more indepth tests. This should fail in lots of places as it is just the tests.

gstegemann’s picture

gstegemann commented
Status: Needs review » Reviewed & tested by the community

I have applied the patch and executed the Web Links Embed tests. The reported the following results

161 passes, 29 fails, 114 exceptions, and 69 debug messages

I cannot check it that matches your results since the tests on d.o. didn't run yet. Second I have seen several PHP notices like "Undefined property", "Undefined offset", "Trying to get property of non-object" and "Invalid argument supplied for foreach()".

jonathan1055’s picture

jonathan1055 commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2564473_20.weblinks_embed_automated_tests_and_code_fix.patch24.75 KB

Yes, there seems to be a hold-up on processing test, see https://qa.drupal.org/pifr/status

Thanks for testing the patch. Those errors are all to be expected, as the Embed functionality is not fully working in the existing code. Here's a patch which include the tests, plus fixes to weblinks_embed.module and a one-line corrections to weblinks.module to allow the text 'unpublished' as a weblinks-embed id (which is attempted in the existing embed process). Other features which I have fixed/improved are:

  • Errors and empty fieldsets are now displayed with their own separate tag
  • these can be styled using css classes .weblinks-embed-error and .weblinks-embed-empty
  • For users who are not allowed to see the unpublished links, the output is generated according to the rules for empty groups, i.e. we now respect the 'empty container text' admin setting
  • the term is checked to see if it is from the weblinks vocabulary
  • informative messages are given if the term is invalid

The patch has some debug in, which you can turn on if you have the devel module. I will remove this before committing anything.

gstegemann’s picture

gstegemann commented
Status: Needs review » Reviewed & tested by the community

I have tested the current patch and it works as described by you.

Just one question:

For users who are not allowed to see the unpublished links, the output is generated according to the rules for empty groups

I.e. when the user has not the "Administer Web Links" permission, right?

jonathan1055’s picture

jonathan1055 commented

Thanks for testing.
That's a good questions regarding the permission. I had assumed that if the user had one of the core permissions 'Bypass content access control' or 'Administer content' then they would be able to (quite correctly) view the unpublished Web Links content. But it seems they cannot, I've just tested it. That may be worth looking at as a separate issue, not here. So yes, at the moment it is the same as not having 'Administer Web Links' permission. The main thing I wanted to avoid was the php errors which were created when trying to get the properties of an obejct which had not been created because the anonymous user could not see the weblinks.

gstegemann’s picture

gstegemann commented

That may be worth looking at as a separate issue, not here.

Yes.

So yes, at the moment it is the same as not having 'Administer Web Links' permission.

I think that is OK for the moment.

  • jonathan1055 committed 41da466 on 7.x-1.x 
    Issue #2564473 by jonathan1055: Weblinks-embed filter settings cannot be...

The last submitted patch, 18: 2564473_18.weblinks_embed_automated_tests_only.patch, failed testing.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 20: 2564473_20.weblinks_embed_automated_tests_and_code_fix.patch, failed testing.

jonathan1055’s picture

jonathan1055 commented
Status: Needs work » Fixed

Ha! The test have suddenly awoken and fail because I've just done the commit.
Thanks for reviewing and testing. I remembered to remove all the debug before committing.

I discovered that setting the cache value to TRUE causes the automated tests to fail with
"Maximum function nesting level of '256' reached, aborting! in /includes/entity.inc on line 1270" so I put a note in the code and left it at FALSE.

jonathan1055’s picture

jonathan1055 commented

Status: Fixed » Closed (fixed)

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