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

[Bug] Routing issues after being able to disable plugins #488

Closed
pupi1985 opened this issue Jan 23, 2017 · 6 comments
Closed

[Bug] Routing issues after being able to disable plugins #488

pupi1985 opened this issue Jan 23, 2017 · 6 comments
Labels
bug Confirmed bug

Comments

@pupi1985
Copy link
Contributor

I noticed an awkward behaviour in some modules that have overridden load_module, particularly in the urltoroot parameter passed to that method. After debugging a lot, I got the the conclusion that accessing the database early in the bootstrap process is messing again with the core.

Now the issue is related to routing. To clearly detect the issue you can echo the $qa_root_url_relative variable in its setter and its getter and you'll find that it is being GET before being SET. It seems the access to the database that detects which plugins should be loaded generates a connection "event" that can be caught by process modules. So all modules loaded so far are scanned at connection time by the method qa_report_process_stage(). This shouldn't be an issue: all modules that are loaded can react, the pending ones can't, and it makes sense.

However, modules are being LOADED to fire that "event", while before they were just REGISTERED when requiring the qa-base.php file. In order to properly set the urltoroot for a module the $qa_root_url_relative global variable needs to be set. There are a couple of places that set that variable. The most obvious one is the qa-index.php file, where the issue is pretty clear: it first requires the qa-base.php file, then modules are loaded (using the empty $qa_root_url_relative) and then the $qa_root_url_relative is set.

At this point my brain got fried. However, I made a quick test for something that seemed it should be a workaround: in qa-index.php I added qa_initialize_plugins(); below the qa_index_set_request() call to make sure the plugins are initialized after the routing is setup and removed it from the qa-base.php file. It worked. The thing is that this would mean always calling a function after requiring qa-base.php to load the plugins.

Another option I haven't thought in detail would be removing the functions in qa-base.php (not the execution logic inside the file) and create a qa-base-functions.php file with them .That way other scripts can import and use the functions (particularly the routing ones) before including the qa-base.php file. Also the qa-base.php file (apart from having the execution logic) should import those functions too so that it would be possible to keep including the file in the same way as it is currently done in order to load the core.

I didn't want to create a PR before hearing your thoughts on this as any approach should mean a change in sensitive files. That's it, I hope this made a bit of sense and save some debugging time in your end.

@svivian svivian added the bug Confirmed bug label Mar 7, 2017
@svivian
Copy link
Collaborator

svivian commented Mar 7, 2017

I noticed an awkward behaviour in some modules that have overridden load_module, particularly in the urltoroot parameter passed to that method.

Do you have an example of a plugin where the issue occurs? Or just some example code. I'm not seeing the issue you're describing. Which functions do you mean by "getter and setter"? I'm assuming qa_set_request and qa_path_to_root

There is an issue with accessing the DB early on as explained in #492, so there are a few issues to resolve. I'm sure we can figure out the best solution here!

Another option I haven't thought in detail would be removing the functions in qa-base.php (not the execution logic inside the file) and create a qa-base-functions.php file with them.

Whether or not that solves this specific issue, it would be a good idea to do anyway. It's regarded as best practice for PHP files to either execute logic or define functions/classes, not both. In fact I already did it with qa-page.php.

@svivian
Copy link
Collaborator

svivian commented Mar 7, 2017

Managed to replicate the issue using the most basic process plugin, qa_example_process class with just load_module function.

From the look of it, moving the qa_initialize_plugins() call works fine. It needs to be added to each of the various request branches though (qa-ajax.php, qa-blobs.php, etc).

I think we'll still have the installation problem from 492, so we'd need to have a process more like:

  1. Set up the request (ie call qa_index_set_request).
  2. Register only the plugins needed before DB init.
  3. Connect to the database using the correct fail handler (so that installation will be triggered if necessary).
  4. Preload all options.
  5. Register plugins that are enabled via the options.

This process would also need to be repeated across each of the request branches.

Edit: another issue, the qa_index_set_request function is overrideable, so initialising plugins after that is gonna cause more problems... I think my brain is getting fried too...

@svivian
Copy link
Collaborator

svivian commented Mar 16, 2017

@pupi1985 Any more thoughts on this? I think 1 and 2 in my list above might need to be swapped.

Alternatively, as was suggested in another thread we could store plugin status in a file rather than the DB.

@pupi1985
Copy link
Contributor Author

Hey Scott. I'm super busy (both with work and in my personal life) these days and I'm pretty sure I will be quite busy for the next couple of weeks. Taking a good look at this will take me a couple of hours and I don't have that time at the moment.

Anyway, IMO, using a file to configure the plugins is closer to a workaround rather than a solution. If the idea is to release a 1.8 alfa version soon, then let's go for that approach. However, IMO, that should be changed in a near future. This will be forcing users who want to disable a plugin to have a writable directory, which is not exactly straight forward for all users and might as well complicate the installation process a bit.

@svivian
Copy link
Collaborator

svivian commented Mar 18, 2017

OK no problem. Thanks for all the effort you're put in so far, really appreciate it. I'll go ahead with my changes and see where that gets us. Fair point about file configuration, but it may be useful as a last resort.

@svivian
Copy link
Collaborator

svivian commented Mar 24, 2017

After a few hours of digging and moving code around I've finally settled on a solution which ended up pretty simple: 5321f11 & 833153e

Unfortunately it does mean people calling Q2A from external code will need to add a call to qa_initialize_postdb_plugins() to ensure optional plugins are loaded. Not sure there's any way around that since qa-base.php cannot connect to the database due to other places like qa-ajax.php needing their own DB fail handlers.

@svivian svivian closed this as completed Mar 24, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Confirmed bug
Projects
None yet
Development

No branches or pull requests

2 participants