Navigation Menu

Skip to content
This repository has been archived by the owner on Nov 3, 2023. It is now read-only.

Add hooks importTheme and exportTheme #7341

Merged

Conversation

ausi
Copy link
Member

@ausi ausi commented Sep 26, 2014

As discussed in #7296 but I have changed the hook parameters.

@leofeyer leofeyer added this to the 3.4.0 milestone Sep 26, 2014
@leofeyer
Copy link
Member

For the sake of completeness, this has been discussed so far:

Regarding the hooks in the Theme class: why did you place them at the very end, after all the logic has been executed? Wouldn't it make more sense to execute them before the XML file is being processed?

a) I wanted to keep the hooks consistent. Both, import and export get the same parameters
b) $xml is an instance of DOMDocument so you can modify the xml file as you like. It's better to put it after the core logic because that allows you e.g. to remove a table completely or add attributes to it etc.

Yes, and then those new attributes are not processed, because the routines have been executed before. Does not make much sense, does it?

In both cases, the core routine is executed first and you can then do whatever you like in your hooks?

But you potentially have to copy the whole routine then (e.g. the automatic UUID conversion) to process your own stuff.

Yes, I know. That's why e.g. UUID conversion should be part of its own component and reusable ;-) (Database\Updater::convertSingleField() might help :-)). I really think the hooks are well placed where they are now.

Well, I don't. (No hard feelings.) Needs to be discussed then.

@leofeyer
Copy link
Member

As discussed on Mumble, it is important that the database import has been completed by the time the hook is executed.

@leofeyer
Copy link
Member

The argument order is currently not consistent, is it?

// importTheme hook
System::importStatic($callback[0])->$callback[1]($xml, $objArchive, $arrMapper);

// exportTheme hook
System::importStatic($callback[0])->$callback[1]($objTheme, $xml, $objArchive);

@ausi What about making $xml and $objArchive the first and second argument in both hooks?

@ausi
Copy link
Member Author

ausi commented Sep 26, 2014

You are right, it was not consistent. I added $objTheme to the importTheme hook as well, because it is very important to know which theme gets imported.

@ausi
Copy link
Member Author

ausi commented Sep 26, 2014

While looking at these theme hooks, I realized that the theme import and export process consists of three steps:

  1. Export the theme
  2. Compare the uploaded cto file with existing files and themes
  3. Import the theme if the compare step was confirmed

If an extension uses the import theme hook, it should be possible to display a warning in the compare step if it cannot handle the imported file correctly. To make that possible I added a third hook called compareTheme. I’m not sure about the name, alternatives: importThemeConfirm, confirmTheme, confirmUploadedTheme or compareThemeFiles.

@leofeyer
Copy link
Member

Since I usually name the hooks after the methods they are triggered in, I would prefer compareThemeFiles. (I also realize that based on this logic, the importTheme hook would have to be called extractThemeFiles.)

And do we really need to pass $objTheme to the callback? The theme ID is available in $arrMapper and if someone needs the model, they can execute ThemeModel::findByPk(reset($arrMapper['tl_theme'])) themselves, can't they?

@ausi
Copy link
Member Author

ausi commented Sep 29, 2014

I think the $objTheme parameter would be very helpful for extension developers. The performance of one database query should also be no problem in the theme import process. If $objTheme is not OK for you, we should at leaset pass the ID of the theme as $intThemeId. So an extension developer doesn't need to understand $arrMapper.

@leofeyer
Copy link
Member

Ok, the theme ID seems like a good compromise. Can you adjust the PR one last time please?

@ausi
Copy link
Member Author

ausi commented Sep 29, 2014

@leofeyer done

leofeyer added a commit that referenced this pull request Sep 30, 2014
@leofeyer leofeyer merged commit e1236f8 into contao:develop Sep 30, 2014
leofeyer added a commit that referenced this pull request Oct 31, 2014
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants