Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Hello World! 2.0 #1296

Closed
Sama34 opened this issue Aug 23, 2014 · 59 comments
Closed

Hello World! 2.0 #1296

Sama34 opened this issue Aug 23, 2014 · 59 comments
Assignees
Milestone

Comments

@Sama34
Copy link
Contributor

@Sama34 Sama34 commented Aug 23, 2014

I think we need to update the example plugin file provided within the pack. It is very outdated.

PR #1389

@Sama34 Sama34 added this to the 1.8.1 milestone Aug 23, 2014
@avril-gh
Copy link
Contributor

@avril-gh avril-gh commented Aug 23, 2014

This ones good idea @Sama34

@Eldenroot
Copy link
Contributor

@Eldenroot Eldenroot commented Aug 23, 2014

What do you wanna update? It is really simple plugin, maybe just information in header?

EDIT: I see, problem with CSS etc. My bad, I am sorry...

@avril-gh
Copy link
Contributor

@avril-gh avril-gh commented Aug 23, 2014

@Cu8eR @Sama34
some examples of how properly add / remove tables, how make changes in template, add / remove custom option, ect so person who will try to make plugin will not reinvent wheel and create own functions without knowing that there are allready functions for everything ready to be used, and whats more - compatible and proper way.

@ghost
Copy link

@ghost ghost commented Aug 23, 2014

Bulk of comment lines to make it a mini plugin guidebook? Not a bad idea!!!
Lets start writing one ...

I've seen complains about MyBB plugin guides many times. Atleast we will have something in core to guide people.

@avril-gh
Copy link
Contributor

@avril-gh avril-gh commented Aug 23, 2014

mini plugin guidebook? Not a bad idea!!!

Thats what 'hello world' is for. Not a big example but something to give easy and whats more important - faultless start without reinventing something that is allready defined.

@ghost
Copy link

@ghost ghost commented Aug 23, 2014

I have example function chunks written in my sublime snippets.
I hope I can contribute few lines.

@avril-gh
Copy link
Contributor

@avril-gh avril-gh commented Aug 23, 2014

Maybe should outline first what 'hello world' will exacly do.
Should be something simple but enought to demonstrate basic techniques.

@Eldenroot
Copy link
Contributor

@Eldenroot Eldenroot commented Aug 23, 2014

@DEMONATE - so you will assignee this to you?

@JN-Jones
Copy link
Contributor

@JN-Jones JN-Jones commented Aug 23, 2014

I vote for removing the plugin and instead improve our plugin docs.

@Eldenroot
Copy link
Contributor

@Eldenroot Eldenroot commented Aug 23, 2014

@JN-Jones - yes, even better solution - many admins just deleted this plugin after MyBB is installed. Personally I have never activated it in last 3 years. Better to spend this time to improve plugin docs

@avril-gh
Copy link
Contributor

@avril-gh avril-gh commented Aug 23, 2014

(...) improve our plugin docs.

btw, what happened to docs.mybb.com/sourcedocs/

@ghost
Copy link

@ghost ghost commented Aug 23, 2014

so you will assignee this to you?

I'm not a collaborator ...

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Aug 23, 2014

docs should be improved of course, I already reported some problems internally.

But I'd leave the example plugin in the package too. Pirata Nervo's looks a lot better than this one so it can replace the current one, not sure if anything should be added to it (setting peekers with the new hook or whatever).

@avril-gh
Copy link
Contributor

@avril-gh avril-gh commented Aug 23, 2014

http://community.mybb.com/thread-110187.html
http://community.mybb.com/thread-110051.html

@PirataNervo woah, brilliant bull's eye 💯

could be not bad idea to rename them to example_plugin_simple and example_plugin_advanced
and include both (hello world and this)

Sure, its true that most of people delete it but thats the ones who arent interested in any moding.
(optionally could drop these 2 plugins as 'example.zip' to plugins folders)
On other side i commonly see many people who would like to mod something but they seem to have completly no clue where to start and what options are available so they hack core files straight from beginning. (even more if they just came from phpBB where its 'normal').

@fabiomaia
Copy link

@fabiomaia fabiomaia commented Aug 23, 2014

btw, what happened to docs.mybb.com/sourcedocs/

I'll be adding that back in soon.

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Aug 23, 2014

@PirataNervo's references was what was in my mind TBH.

A complete example plugin showing the right way to use the plugin system without relying on external libraries, etc.

The code in the plugin should be used in the docs and the docs should document what is within the plugin file.

No sure about a bunch of comment lines in the file, that is what the docs are for.

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Sep 5, 2014

Please give feedback (:

@Sama34 Sama34 self-assigned this Sep 5, 2014
@avril-gh
Copy link
Contributor

@avril-gh avril-gh commented Sep 5, 2014

Nice, for sure better than old hello world.

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Sep 5, 2014

Please give feedback (:

  1. Setting are usually handled by activate/deactivate functions. I think they should be in this plugin too.
  2. There are some unnecessary globals, for example $mybb in install, is_installed and uninstall.
  3. The message input should be trimmed before empty() check and get_input() should be used.
  4. Keep the code as simple and readable as possible. So:
/* $db->delete_query('settings', 'name IN (\'hello_display1\',\'hello_display2\')'); // just no, why the 4 escapes when " can be used? */
$db->delete_query('settings', "name IN ('hello_display1', 'hello_display2')");

or:

/* eval("\$hello = \"".$templates->get('hello_index')."\";"); // just no, why not use ' or the new render() function? */
eval('$hello = "'.$templates->get('hello_index').'";'); // or
$hello = eval($templates->render('hello_index'));
@JN-Jones
Copy link
Contributor

@JN-Jones JN-Jones commented Sep 5, 2014

About 1: I add/remove my settings always in install and IMHO they should be handled by them. If you deactivate the plugin you only want that it isn't displayed anymore, but all settings/all data should be kept. So activate/deactivate should only handle template edits. Just my opinion

@avril-gh
Copy link
Contributor

@avril-gh avril-gh commented Sep 6, 2014

If you deactivate the plugin you only want that it isn't displayed anymore, but all settings/all data should be kept.

i think the same. Thats exacly why there are activate / deactivate. Else would be only install / uninstall

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Sep 6, 2014

Agree with @JN-Jones. If you do it in activate/deactivate you are doing it wrong. Ideally something like what PluginLibrary does should be used for handling updates to settings. But there is no core features for it.

We may write some for this plugin only or enhance the core.

Also, doing it this way avoids the use of "Disable/Enable this plugin whatever" kind of settings.

4 If we added the 'render()' method then I agree to use it.

just no, why the 4 escapes when " can be used?

I don't agree (kind of) but since it is on our standards I shall update that then, I suppose.

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Oct 19, 2014

http://community.mybb.com/thread-161297-post-1110751.html#pid1110751 - exemplary codename with a descriptive comment should be added too.

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Oct 20, 2014

Yeah, I kind of guessed it from the beginning. I think it was clear in the internal discussion but yet.

@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Nov 18, 2014

@Sama34 can you get this in time for 1.8.3?

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Nov 18, 2014

I have updated the plugin to implement suggestions, settings now got inserted/updated during activation and removed during uninstallation. Using PluginLibrary approach.

It also uses the language system as best as possible in the ACP.

Templates are a complicated thing nonetheless. We could also store messages in the cache so that people know how to actually use the cache system. Going to implement an groupselect setting too, so that people know to use the is_member() function.

Please suggest what could be enhanced without really adding new or complicated features. Remember this is not meant to be a simple plugin but one that actually teach people what is available there and how to make the best use of them.

@mybb/developers @Destroy666x @JN-Jones

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Nov 21, 2014

I have added a confirmation page to the uninstallation routine.

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Dec 1, 2014

I have added PluginLibrary implementation of the tempalte handling. I need this merged before jumping to anything else so please give feedback if you have any.

I have also though about adding an ACP side for managing messages (currently there is not anyway to do so) but that would require a new file (proper way) and that would make it too complicated for just an test plugin.

@mybb/developers @Destroy666x @JN-Jones

@JN-Jones
Copy link
Contributor

@JN-Jones JN-Jones commented Dec 2, 2014

IMHO this should be a simple plugin to show how MyBB (and the plugin system) work and not how a plugin works. If someone wants to know how one specific thing works he can look into a plugin which does something similar. Also we shouldn't use PluginLibrary IMHO - it's a nice tool I've used too, but I don't see the point in using it in an example plugin.

I have added a confirmation page to the uninstallation routine.

Why that? I've never seen a plugin doing that (and tbh I'd never use one doing that)

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Dec 3, 2014

Does anyone actually know how to add one? Probably that is the reason no one ever uses it. I have seem only advanced plugind using one, I think it is a nice addition.

Also, this doesn't use PluginLibrary, it just uses its was to add a template group and handling settings.

@Stefan-ST
Copy link
Contributor

@Stefan-ST Stefan-ST commented Dec 3, 2014

I don't think the example plugin should cover such rare cases.

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Dec 4, 2014

Agreed with @JN-Jones and @Stefan-ST , never seen anyone adding a confirmation page so I don't think we should show that in an example. It'd be better to introduce it to core for every uninstallation link rather than in several single plugins.

Also, found some small issues, will post them later.

@Eldenroot
Copy link
Contributor

@Eldenroot Eldenroot commented Dec 4, 2014

This plugin should show all possibilities - we should leave a confirmation dialog... I dont see any problem with this

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Dec 4, 2014

This plugin should show all possibilities

First of all, there is no term "all possibilities" in programming.. Showing "everything" would require an infinite number of code and time, not to mention that noone would use it as an example since it would be longer than all the core files merged into one..

Secondly, I can name many many things that are not part of this exemplary plugin while they're used definitely more often than uninstall confirmations, for instance setting peekers, cookies, AJAX, adding stylesheets, group/forum additional settings and so on.

@Stefan-ST
Copy link
Contributor

@Stefan-ST Stefan-ST commented Dec 4, 2014

@Cu8eR An example cannot cover all possible cases. It should show common things (which are used by a lot of plugins). A confirmation dialog is not common.

@Eldenroot
Copy link
Contributor

@Eldenroot Eldenroot commented Dec 7, 2014

I agree, but why to remove it if it has been already added (yes, it is not common function, it should not be there but it has been already added).. just comment it properly in code and leave it. Thats my opinion.

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Dec 7, 2014

Anyways, here are the small bugs I wanted to list few days ago:

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Dec 17, 2014

I have used error_no_permission() for demonstrative uses only because it is not a permission issue neither users should access the feature if not by post request.

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Dec 17, 2014

I have updated the confirmation page so if anyone can please confirm this ready to be merged please do so. If removing the confirmation page is required to get this merged then so be it, I just need this tested to merge it. Thanks.

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Dec 17, 2014

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Dec 17, 2014

2 more issues:

  1. $templatelist doesn't have much sense now, you cache the hello_message template on every page with defined THIS_SCRIPT while it's needed only in showthread/index. I'd simple remove $templatelist .= ', hello_message'; and add the template to both scripts:
    if(THIS_SCRIPT== 'index.php')
    {
        $templatelist .= 'hello_index,hello_message';
    }
    elseif(THIS_SCRIPT== 'showthread.php')
    {
        $templatelist .= 'hello_post,hello_message';
    }
  1. The whole $post['message'] is replaced with the messages now while they should be concantenated as earlier.
@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Dec 17, 2014

I just forgot what $post['message'] was about, lol

I fixed both issues.

@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Dec 19, 2014

@Destroy666x not sure about now but before, 16* would be interpreted as any 1.6 version. I believe it's still the same for 18*.
Every other issue you brought up seems good to me.

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Dec 19, 2014

@PirataNervo , but you wrote it without a dot too, that's what I meant, earlier it was 1.8*. It's already corrected.

@DiogoParrinha
Copy link
Contributor

@DiogoParrinha DiogoParrinha commented Dec 19, 2014

Ah sorry I couldn't see that in the link anymore :P

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Dec 19, 2014

So are you up for merging this?

@Destroy666x
Copy link
Contributor

@Destroy666x Destroy666x commented Dec 19, 2014

Last thing I'd change:

A sample plugin that prepends the messages in each post.

to a more descriptive:

A sample plugin that allows you to create messages on the index page and appends them to each post.

And maybe the weird huge tabbing of the single array: https://github.com/mybb/mybb/pull/1389/files#diff-2dc20e060a386c409ac7f6f95cc0c623R279

Then I think it can be merged.

Anything else (like AJAX) can be added in another PR if needed.

@Sama34
Copy link
Contributor Author

@Sama34 Sama34 commented Dec 21, 2014

If that is it then I will merge it now. Thanks.

Sama34 added a commit that referenced this issue Dec 21, 2014
@Sama34 Sama34 closed this Dec 21, 2014
@Sama34 Sama34 added s:resolved and removed s:feedback labels Jul 8, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants
You can’t perform that action at this time.