search

pkp / textEditorExtras

An OJS plugin to add controls to the rich text editor to upload images, manipulate the HTML code, and add tables.
GNU General Public License v3.0
3 stars 14 forks source link

initData Correction : error in template, so initialize with empty values #19

Closed mathiasv27 closed 1 year ago

mathiasv27 commented 1 year ago

Hi, I've made a little correction in iniData method. An error occured in 3.3.0.14, in settings.tpl : PHP Fatal error: Uncaught TypeError: in_array(): Argument #2 ($haystack) must be of type array, null given So the "additions" var is initialized with empty arrays if datas are not present.

NateWr commented 1 year ago

Thanks @mathiasv27! It looks like your code editor has applied formatting across the file. Can you redo your PR so that only the bug fix is applied?

Also, can you remove the changes to the French .po file? All translations are applied through weblate. You can make changes for this plugin here: https://translate.pkp.sfu.ca/projects/plugins/texteditorextras/

mathiasv27 commented 1 year ago

Ok, thanks ! I've reverted my commit and pushed only the bug fix, is that ok or do I have to cancel this request and redo a PR ?

NateWr commented 1 year ago

That's all my comments addressed. @mathiasv27 are you ready for me to merge this and port it to main?

mathiasv27 commented 1 year ago

It's ok for me !

mathiasv27 commented 1 year ago

Hi, Is it possible to merge, or is something missing ? Thanks a lot !

asmecher commented 1 year ago

@mathiasv27, I think this is actually a duplicate of https://github.com/pkp/pkp-lib/issues/8064, which has already been fixed with additions to the template (rather than the .php code). What version of the plugin are you using?

mathiasv27 commented 1 year ago

I'm using the last version on stable-3_3_0 : 1.0.0.2 . The additions to the template, such as if is_array($additions.masthead.description) didn't work, because if $additions or $additions.masthead is not initialized as an array, it throws an error...

asmecher commented 1 year ago

@mathiasv27, ah! I think that's a mistake in using is_array instead of isset. Using isset instead would suppress the warning. Does that solve the problem in your case?

Your proposed solution of pre-initializing in PHP is also 100% valid, but with the additions already started in the template side, I'd favour continuing there. If you do prefer the PHP-side approach, please feel free to update the PR, but do also remove the template is_array calls in the process.

mathiasv27 commented 1 year ago

Ok, that's better: I replaced is_array by isset and removed the is_array tests in the template. I prefer this approach indeed: sending a formatted and tested array of values to the template is, from my modest point of view, preferable to doing existence tests in a template :) Thank you so much ! We have to present to users on our staging server shortly, do you think this can be merged soon?

asmecher commented 1 year ago

@mathiasv27, agreed, this is a better solution. Thanks for working through it with us (and sorry for the delay)! I've merged it to stable-3_3_0 and forward-ported it to main for the forthcoming 3.4.0 branch.

mathiasv27 commented 1 year ago

@mathiasv27, agreed, this is a better solution. Thanks for working through it with us (and sorry for the delay)! I've merged it to stable-3_3_0 and forward-ported it to main for the forthcoming 3.4.0 branch.

Thanks a lot, Alec ! Don't worry about the delay, I understand ;)