Replace deprecated code; bug fix #299

Miro-Collas · 2023-08-18T19:33:14Z

Attempts to resolve

Note that no other clean-up or change was attempted.

Attempt to fix #297

attempt to fix #298

fiwswe · 2023-08-18T19:48:48Z

helper.php

@@ -550,10 +552,12 @@ private function adapt_links(&$ins, $page, $included_pages = null) {
            // adjust links with image titles
            if (strpos($ins[$i][0], 'link') !== false && isset($ins[$i][1][1]) && is_array($ins[$i][1][1]) && $ins[$i][1][1]['type'] == 'internalmedia') {
                // resolve relative ids, but without cleaning in order to preserve the name
-                $media_id = resolve_id($ns, $ins[$i][1][1]['src']);
+                $resolver = new PageResolver($ns);


Are you sure a PageResolver object is what you want here? Seems to me for media a MediaResolver object would make more sense.

(I looked at this and did not submit a PR because quite frankly I'm not sure I fully understand the implications. So take my question with that in mind.)

Are you sure a PageResolver object is what you want here? Seems to me for media a MediaResolver object would make more sense.

(I looked at this and did not submit a PR because quite frankly I'm not sure I fully understand the implications. So take my question with that in mind.)

To be blunt, I don't really understand it either. My OCD kicked in and I am trying to clean stuff since the dokuwiki logs are full of warnings.

The code seems to work, I have it live on our wiki. But I can try what you suggested.

Looking at the examples on
https://www.dokuwiki.org/devel:releases:refactor2021#refactoring_of_id_resolving
suggests that the function there should have been resolve_mediaid(); instead it is resolve_pageid(). resolve_pageid() is replaced by PageResolver.
You may well be right but given that I have the code working (at least for my use of it), I'm going to leave it as-is and let the original author correct it, if it needs to be.

The PageResolver does currently more then the MediaResolver, so I expect it could work indeed… If here only media are handled, it could be confusing in the future that these are mixed. My preference would be to use the specific resolver for each.

There is no non-deprecated equivalent to the resolve_id function. It might be that the abstract resolver parent class provides the code and could be used as a parent class of a new resolver class in the include plugin, otherwise the code of the old resolve_id function needs to be copied to a helper method in the include plugin.

As the comment above this code explains, we need a resolver that doesn't clean the id. Otherwise, link labels won't be the same. For example, linking to {{Hello World.pdf}} should produce a link with the label "Hello World", this is impossible if the id is cleaned to "hello_world.png". Both the media and page resolver always clean the id. Same for links to pages, try, e.g., [[Hello World]]. I suspect that for this reason also many of the tests fail but I haven't looked at the details.

True, either I didn't consider that for media files or it is indeed not a problem. The resolver will be called again in the renderer, here we really just need to make relative ids absolute so they work independently of the current page.

I think this discussion could be answered by installing the modified code and seeing if it behaves as expected, no? It does for me. Then again, I may not be covering all possible use cases. Still, an empirical test would determine whether the changes are correct or not.

Create a page wiki:included with content

* [[test|{{dokuwiki.png}}]] * [[#test|{{dokuwiki.png?w=200}}]] * [[doku>test|{{dokuwiki.png?w=300}}]] * [[test|{{https://www.dokuwiki.org/lib/tpl/dokuwiki/images/logo.png}}]]

and a page test:include with content {{page>..:wiki:included}} (this is the setup that is tested in one of the failing tests). This should display the DokuWiki logo in every list item. According to the test result, the image is only displayed in the last item.

I think there is a bug in the change, the old code assigned $media_id while the new code directly assigns the result to the instruction but then assigns $media_id to the instruction, thereby removing the media id from the instruction, which would explain the test failure.

If you want to test what we discussed here, change the code to reference DokuWiki.png and compare the alt text and title attribute of the image (not sure) in the original and included version. Alternatively, upload a pdf file and link to it with an uppercase file name and check if the case is preserved.

Create a page wiki:included with content

OK I did that (after changing it to use an image that exists ion my install), and can confirm in part.

I'll be more than happy to make any changes you propose.

Create a page wiki:included with content

* [[test|{{dokuwiki.png}}]] * [[#test|{{dokuwiki.png?w=200}}]] * [[doku>test|{{dokuwiki.png?w=300}}]] * [[test|{{https://www.dokuwiki.org/lib/tpl/dokuwiki/images/logo.png}}]]

and a page test:include with content {{page>..:wiki:included}} (this is the setup that is tested in one of the failing tests). This should display the DokuWiki logo in every list item. According to the test result, the image is only displayed in the last item.

I think there is a bug in the change, the old code assigned $media_id while the new code directly assigns the result to the instruction but then assigns $media_id to the instruction, thereby removing the media id from the instruction, which would explain the test failure.

If you want to test what we discussed here, change the code to reference DokuWiki.png and compare the alt text and title attribute of the image (not sure) in the original and included version. Alternatively, upload a pdf file and link to it with an uppercase file name and check if the case is preserved.

Seems the above generates several warnings:

2023-08-19 19:40:26E_WARNING: Undefined variable $media_id/home/fswiki/public_html/lib/plugins/include/helper.php(558) #0 /home/fswiki/public_html/lib/plugins/include/helper.php(558): dokuwiki\ErrorHandler::errorHandler(2, 'Undefined varia...', '/home/fswiki/pu...', 558) #1 /home/fswiki/public_html/lib/plugins/include/helper.php(317): helper_plugin_include->adapt_links(Array, 'wiki:test', Array) #2 /home/fswiki/public_html/lib/plugins/include/helper.php(277): helper_plugin_include->_convert_instructions(Array, 0, 'wiki:test', NULL, Array, 'support:test1', Array) #3 /home/fswiki/public_html/lib/plugins/include/syntax/include.php(150): helper_plugin_include->_get_instructions('wiki:test', NULL, 'page', 0, Array, 'support:test1', Array) #4 /home/fswiki/public_html/inc/parser/renderer.php(119): syntax_plugin_include_include->render('metadata', Object(Doku_Renderer_metadata), Array) #5 /home/fswiki/public_html/inc/parserutils.php(540): Doku_Renderer->plugin('include_include', Array, 5, '{{page>:wiki:te...') #6 /home/fswiki/public_html/inc/parserutils.php(299): p_render_metadata('support:test1', Array) #7 /home/fswiki/public_html/inc/common.php(266): p_get_metadata('support:test1') #8 /home/fswiki/public_html/doku.php(97): pageinfo() #9 {main} 2023-08-19 19:40:26E_WARNING: Trying to access array offset on value of type null/home/fswiki/public_html/lib/plugins/include/helper.php(558) #0 /home/fswiki/public_html/lib/plugins/include/helper.php(558): dokuwiki\ErrorHandler::errorHandler(2, 'Trying to acces...', '/home/fswiki/pu...', 558) #1 /home/fswiki/public_html/lib/plugins/include/helper.php(317): helper_plugin_include->adapt_links(Array, 'wiki:test', Array) #2 /home/fswiki/public_html/lib/plugins/include/helper.php(277): helper_plugin_include->_convert_instructions(Array, 0, 'wiki:test', NULL, Array, 'support:test1', Array) #3 /home/fswiki/public_html/lib/plugins/include/syntax/include.php(150): helper_plugin_include->_get_instructions('wiki:test', NULL, 'page', 0, Array, 'support:test1', Array) #4 /home/fswiki/public_html/inc/parser/renderer.php(119): syntax_plugin_include_include->render('metadata', Object(Doku_Renderer_metadata), Array) #5 /home/fswiki/public_html/inc/parserutils.php(540): Doku_Renderer->plugin('include_include', Array, 5, '{{page>:wiki:te...') #6 /home/fswiki/public_html/inc/parserutils.php(299): p_render_metadata('support:test1', Array) #7 /home/fswiki/public_html/inc/common.php(266): p_get_metadata('support:test1') #8 /home/fswiki/public_html/doku.php(97): pageinfo() #9 {main}

Again, I'll be happy to fix the code in any way you might suggest - assuming it is tested. [grin]

helper.php

action.php

helper.php

@fiwswe

ref #299 (comment) - with thanks to @fiwswe

michitux

As explained in previous comments, the new resolver classes provide no replacement for resolve_id, unfortunately. Therefore, this replacement needs to be added first in the include plugin.

michitux · 2023-08-19T10:56:06Z

helper.php

-                            resolve_pageid($ns, $link_id, $exists);
+                            $resolver = new PageResolver($ns);
+                            $link_id = $resolver->resolveID($link_id);
+                            $exists = page_exists($link_id);


I don't see where this $exists variable is used, I think it can simply be removed.

helper.php

michitux · 2023-08-19T10:59:50Z

helper.php

@@ -567,20 +571,23 @@ private function adapt_links(&$ins, $page, $included_pages = null) {
                        $link_params = $link_parts[1];
                    }
                    // resolve the id without cleaning it
-                    $link_id = resolve_id($ns, $link_id, false);
+                    $resolver = new PageResolver($ns);
+                    $link_id = $resolver->resolveID($link_id);


Same here as above, PageResolver is not the same as resolve_id, this is breaking link labels in some cases.

I have just been following what it says on
https://www.dokuwiki.org/devel:releases:refactor2021#refactoring_of_id_resolving
what do you suggest doing instead?

Miro-Collas · 2023-08-21T06:00:57Z

Having thought about it, I am closing this PR. It evidently contains a bug which I don't know how to resolve.
Thanks to all who commented and suggested changes - I really appreciate it! Apologies for any time wasted.

Miro-Collas added 2 commits August 18, 2023 15:29

Replaced deprecated resolve_pageid()

ea5f922

Attempt to fix #297

Bugfix

dc54f91

attempt to fix #298

fiwswe reviewed Aug 18, 2023

View reviewed changes

Improvement

f36e747

ref #299 (comment) - with thanks to @fiwswe

michitux requested changes Aug 19, 2023

View reviewed changes

Remove unused variables

3a7648c

Miro-Collas requested a review from michitux August 21, 2023 00:42

Miro-Collas closed this Aug 21, 2023

Miro-Collas mentioned this pull request Sep 22, 2023

Deprecation Warnings after upgrade to 2023-04-04a "Jack Jackrum" #297

Open

michitux mentioned this pull request Sep 22, 2023

Fix #292 #297 deprecated id resolving functions #300

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace deprecated code; bug fix #299

Replace deprecated code; bug fix #299

Miro-Collas commented Aug 18, 2023

fiwswe Aug 18, 2023

Miro-Collas Aug 18, 2023

Miro-Collas Aug 18, 2023

Klap-in Aug 19, 2023

michitux Aug 19, 2023

michitux Aug 19, 2023

Miro-Collas Aug 19, 2023

michitux Aug 19, 2023 •

edited

Loading

Miro-Collas Aug 19, 2023

Miro-Collas Aug 19, 2023

michitux left a comment

michitux Aug 19, 2023

Miro-Collas Aug 20, 2023

michitux Aug 19, 2023

Miro-Collas Aug 20, 2023

Miro-Collas commented Aug 21, 2023 •

edited

Loading

Replace deprecated code; bug fix #299

Replace deprecated code; bug fix #299

Conversation

Miro-Collas commented Aug 18, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michitux Aug 19, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michitux left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Miro-Collas commented Aug 21, 2023 • edited Loading

michitux Aug 19, 2023 •

edited

Loading

Miro-Collas commented Aug 21, 2023 •

edited

Loading