IDE-assisted bug fixes - part 3

timschofield / webERP

webERP Accounting and Business Administration ERP system

https://www.weberp.org

GNU General Public License v2.0

29 stars 113 forks source link

IDE-assisted bug fixes - part 3 #196

Closed aingelc12ell closed 1 month ago

aingelc12ell commented 1 month ago

Fix: css identifiers and terms
Fix: Private visibility access via __get()
Fix: allow strict https implementation be using // instead of http://

timschofield commented 1 month ago

Thanks for this. It is great, except I'm unsure about the changes to the prnMsg() function. I see you have created the option to print the message "inline" instead of grouping all the meassages at the top. I'm not against this idea (though I would need to see it in action to be sure) but unless I have missed something, wouldn't the message get duplicated, as it is still being added to the $Messages[] array? prnMsg() used to output inline, but then it was decided it was better to group them together. I think it was @andrewcouling who did the work on this before. Maybe your idea of mixing this is the best idea though.

aingelc12ell commented 1 month ago

Good observation. I too am not sure why in some uses, there are echo preceding prnMsg(). In my understanding, some of the former developers wanted to put those messages right where they are now, then prnMsg() was added where they are grouped together.

After a quick glance into the function, yes, the messages are duplicated.

What would you think will be good to add to the group IF NOT have been returned as inline? I'm ready for another pull request :D

timschofield commented 1 month ago

The more I think about it, the more I like your idea of giving the developer the option of having it echoed inline, or putting it at the top. I think it very much depends on each case. Problem is there are hundreds (thousands??) of instances throughout the whole codebase. I think I would keep your change to the function (just removing the possibility of duplication) but remove all the echoes for now, making a separate project to review the instances of all prnMsg() calls in future. That way we have the facility for developers to choose. What do you think?

aingelc12ell commented 1 month ago

I can make the changes to remove all the echo for now.

And let the new developers choose how they can be implemented.

merge the pull request for now, So I can update my fork from your base

timschofield commented 1 month ago

Ok, have done