Unless I have misunderstood something doesnt this mean that it will only work with the atum template?

Guess the dependency can be declared on the templates ass.json file.

It looks good. You loading another extension, what you made here is fine.

Unless I have misunderstood something doesnt this mean that it will only work with the atum template?

Yes exactly. As for me this feature has a tight relation to the template it has to actively enable it.

Unless I have misunderstood something doesnt this mean that it will only work with the atum template?

Yes exactly. As for me this feature has a tight relation to the template it has to actively enable it.

In that case this is a down vote from me as afaik there is no core functionality that is dependent on the template being used.

Ok, here is my personal opinion:
I agree, there is still stuff which can be improved in the Guided Tour and I really appreciate that you make your thoughts about it, but the criteria which are hold against the Guided Tour (and leading to this PR) are very hypocritical.

No system plugin

Currently you can, if you don't want to have the tour active just disable the module + plugin. With this implementation you're stuck to boot the component always and also the assets are not disabled. Implementing it in the template always leads to a non-disable overhead.

Additional the PR from @obuisard fixes the problem of having stuff loaded in the frontend.

Looking into the plugin list that is the way we do it.

Template binding

This is a move which kills the tour from the start, as it's first not documented and 2nd people don't understand it how to implement it. (Sorry I don't count "there are a few bootstrap classes", they're very minor as shepherd.js is bootstrap independed and I think it falls back to the button classes). If we put that meassurement as a criteria, we really have a problem with e.g. validation.

So this looks for me not the correct solution and it tries to fix things with solutions which just bring in other problems.

As I was digging deeper into the code it became obvious that the asset loading must be in the module. So I changed this and the template is not touched anymore. Now it should be much more logical. @brianteeman can you have a look again, should be better now and more clearer.

At first, I thought using module is a smart solution. However, the problem is that when we are on create article screen for example, the module is not loaded, so the script.... are not loaded and it breaks the feature.

Any idea how to solve that?

Afraid we would have to follow @wilsonge suggestion: Create a new plugin group and import it for administrator app only.

It was hardcoded, that it doesn't show up in forms. Removed that restriction with fa89db6.

That restriction was by design but had consequences #40025

Seem to work but still have some concerns:

• This assumes the module is assigned to a position which is always loaded in the template
• If you are entering data on the form and click on a tour, then the page is redirected and you lost the data entered in the form. Maybe that was the reason it is designed to not being loaded on forms.

I can adapt the code that the assets are loaded but not the button. So we have the same behavior as with the system plugin. And yes, when the module is not loaded, then no guided tours ?‍♂️ .

That restriction was by design but had consequences #40025

With this approach we can enable or disable it and still make tours working correctly.

I can adapt the code that the assets are loaded but not the button. So we have the same behavior as with the system plugin. And yes, when the module is not loaded, then no guided tours

Kind of workaround to me. But we will see how others think about this solution.

I think correct approach it to have a component to handle all requests and all logic, and plugin to load asset.
Because at current point of time we cannot have "single extension" that does both.

For me #40037 is okay (not perfect but okay), I would also move out all startTour logic to component controller (that what Allon made here).

Addittionaly: whole asset should be under media/com_guidedtours not plugin or module, because it part of com_guidedtours

Why do you need a plugin which does that? I'm not getting it? The module starts the tour so it should be responsible for all of it.

Why do you need a plugin which does both?

I did not said that. I wrote about "single extension", eg. component that can listen to App events, kind of.
Well, nevermind, not really important ?

Okay, maybe module is not that bad also.

Makes indeed more sense to have the assets in the component folder. Moved it back.

@laoneo Regarding the last commit to remove the SQL: It is ok to change the existing update SQL script to during an update we will not add that record just for remotving it later. But it also needs a new update SQL script to remove it when updating from a 4.3.0-beta4 to a later beta or RC or stable. That new update SQl should have a time stamp in its name which is newer than 4.3.0-2023-02-25.sql, e.g. 4.3.0-2023-03-08.sql.

@richard67 can you help me with that?

Making a functionality depend on a module load doesn't seems right to me. It's not logical.

@richard67 can you help me with that?

@laoneo DELETE FROM #__extensions WHERE type = 'plugin' AND element = 'guidedtours' AND folder = 'system' AND client_id = 0;

But maybe we should better uninstall it using the installer like it is done here with the EOS plugin:

For the current architecture this pr is absolutely ok as the module is initiating the whole process. Currently when you disable the module all the assets are still loaded which makes no sense at all. When more use cases will be implemented in the future (which I hope as I see a big potential in the guided tours feature) and having the functionality in a module is not anymore enough, then we can discus new approaches. But for now this is fine as it is.

With this module only approach it's not possible to use the guided tour in the frontend

With this module only approach it's not possible to use the guided tour in the frontend

wouldnt you just need a site module?

@laoneo I see you have decided to use an update SQL script to delete the extension and not a new method in script.php for uninstalling it. To be honest I don’t know if that works. We have a system plugin which is deleted from the extensions table with the update SQL script and the files of that plugin will be removed with the deleted files thing in script.php. I don’t know from scratch now in which order of processing that happens. Worst case is we delete the plugin while it still is loaded. Please carefully test an update of a beta 4 where the guided tours are used to the update package of this PR. We should be 100% sure it doesn’t crash. I think there should be a reason why other plugins are uninstalled with a method in script.php instead of doing it with an update SQL. Maybe @wilsonge remembers why we did it that way for other plugins?

With this module only approach it's not possible to use the guided tour in the frontend

Making guided tours available on the front end is a much bigger task as there is no standard way to create the submit buttons etc. So to implement this it needs a much bigger refactoring. As I said, this change is for the current architecture and when we want to expand to the front end, it needs much more logic. Then it would be possible to think about plugins, for a custom group.

wouldnt you just need a site module?

@brianteeman The problem is that the current implementation requires that the module is always loaded which is not always true (especially when it is used on frontend).