dokufreaks / plugin-include

A DokuWiki plugin for including other wiki pages into the current one.
http://www.dokuwiki.org/plugin:include
GNU General Public License v2.0
62 stars 54 forks source link

Undefined index warnings #245

Closed bradbulger closed 2 years ago

bradbulger commented 4 years ago

I am getting several undefined index warnings on the current (2020-04-16) version.

One is a warning about $instruction['flags'] in _get_included_pages_from_meta_instructions() in helper.php. Since flags is an optional component, it seems like it's OK to just use NULL if it's not set: $flags = isset($instruction['flags']) ? $instruction['flags'] : NULL; The others are more obscure, I haven't been able to figure out the ultimate source. The warnings are from Dokuwiki, from html_secedit_button() in inc/html.php. It is referring to an undefined index 'hid' in the section edit button data. I can't tell if it is working without it, or if there is a problem caused by it not being defined. The calls are all from the include plugin: plugin_include_start, plugin_include_editbtn, and plugin_include_end.

michitux commented 4 years ago

The way this $instructions metadata is set here should always include the flags key. We should probably set the hid. This is an explicit id of the header to redirect to after editing. If you figure out where exactly this is missing and to what value we should set it, feel free to open a pull request, otherwise I can try to have a look at it in the next weeks.

bradbulger commented 4 years ago

Yeah it was tricky to trace that back. I will see if I can figure it out.

On Sat, Apr 25, 2020 at 1:57 AM Michael Hamann notifications@github.com wrote:

The way this $instructions metadata is set here https://github.com/dokufreaks/plugin-include/blob/3948ceccc6baa6b41cc4cfc11dd6b36c3687773c/syntax/include.php#L119 should always include the flags key. We should probably set the hid. This is an explicit id of the header to redirect to after editing. If you figure out where exactly this is missing and to what value we should set it, feel free to open a pull request, otherwise I can try to have a look at it in the next weeks.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/dokufreaks/plugin-include/issues/245#issuecomment-619346258, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAJNNSR6TPZEFKFPNFACUGDROKQY5ANCNFSM4MOOCYPA .

bradbulger commented 4 years ago

That line in syntax_plugin_include_include::include() is curious. Using the $flags variable in compact() will only ever return an empty array. It looks for variable names in the values of $flags. I changed that to use 'flags' instead and that undefined key error goes away. $flags is an array of values like 'somename' => 0 at that point, I guess that's OK to be passing here?

I see multiple places where there are list() calls with expectations of how many elements will be in arrays, or how many parts will result from exploding or splitting strings, that frequently aren't met. I can think of different ways to deal with that, like multiple array_shift() calls, or adding a default array to the source.

michitux commented 4 years ago

That line in syntax_plugin_include_include::include() is curious. Using the $flags variable in compact() will only ever return an empty array. It looks for variable names in the values of $flags. I changed that to use 'flags' instead and that undefined key error goes away. $flags is an array of values like 'somename' => 0 at that point, I guess that's OK to be passing here?

Yes, that was the intended behavior that is actually also important for correct cache handling, I have just pushed a fix. I have just fixed two of those list()-calls using array_pad as a tiny step towards fixing those undefined index warnings.