XF does not loop through all possible folders inside '_output' on import/upgrade

[nocte]

Affected version

2.3.6

In certain situations on xf-addon:upgrade or xf-dev:import XF does not remove orphaned data from the database (data, that was removed from the _output folder).

According to my tests we need at least 1 folder inside _output (e.g. phrases). If another folder (e.g. widget_definitions) is deleted, XF does completely ignore that fact on upgrade or import and just skips removing any (in this case) widget definitions from the database.

Result: If you build a release, if will contain orphaned data.

In what situations does this happen?

If you work with other developers (e.g. on Github) and you pull a commit, where a folder inside _output has been removed, you may face this problem.

The removal may happen, if there used to be 1 widget definition (or other thing, like a route, ..) and this was removed via admin cp. The only file left in the folder is then _metadata.json, which is often ignored via .gitignore. And empty folders are not pushed to Github.

How to reproduce:

1. Create a dummy add-on: php cmd.php xf-addon:create.
2. Create a widget definition (just use XF\Widget\Hmtl as class for testing purpose). You will now have a phrases and a widget_definitions folder in your _output folder.
3. Delete the widget_positions folder.
4. Run php cmd.php xf-addon:upgrade Test/Addon --force.

Result: The widget definition is still in the database and visible in the admin cp.

Expected behavior: The widget definition should be removed.

---

TL;DR: It seems that XF only loops through exiting folders inside _output, when doing an import/upgrade. If a folder is missing, XF does not remove the data belonging to the missing folder. Only exception: If there's not a single folders inside _output, everything seems to be removed and this problem does not occur.

Solution: XF should loop through all possible folders inside _output (not only the existing ones) on import/upgrade and if one does not exist remove all data connected to that folder.

 

[Chris D]

The only file left in the folder is then _metadata.json, which is often ignored via .gitignore.

We'll give this some thought but ultimately the _metadata.json file is not supposed to be ignored, partially for this reason.

At the very least, that's the current workaround to this situation.

 

[Xon]

Committing _metadata.json causes endless merge conflicts, the format is practically made to cause issues. Plus it causes endless pointless churn which makes git change logs harder to work with

IMO it just isn't needed if you include a few extra CLI commands.

I've got a /usr/local/bin/xf-addon-import script which has this contents:

#!/bin/bash

if [[ "$#" -ne 1 ]]; then
    echo "Require an add-on name"
    exit 1;
fi

if [[ ! -d "src/addons/$1" ]]; then
    echo "Expect src/addons/$1  to exist"
    exit 1;
fi

php cmd.php xf-dev:metadata-clean -v || exit 1
php cmd.php xf-dev:import --addon="$1" -v || exit 1
php cmd.php xf-dev:rebuild-caches -v || exit 1

This forces XF to discard missing files, and then it does the import. For some reason the actions in xf-dev:rebuild-caches aren't done in the xf-dev:import command.


I have an xf-addon-import-export which does the import, and then generate-finders and then export actions. This round-tripping is useful as it catches some error cases.

 

[nocte]

Thank you for your feedback, Chris!

the _metadata.json file is not supposed to be ignored, partially for this reason.

Well, but that's what most XF devs are doing.

Here are just 2 examples:

SeederForXF2/.gitignore at master · ticktackk/SeederForXF2

This add-on helps in filling development board with dummy data. - ticktackk/SeederForXF2

github.com

XenForo2-StandardLib/.gitignore at main · Xon/XenForo2-StandardLib

A number of helper utilities designed to ease add-on development - Xon/XenForo2-StandardLib

github.com

I found this issue, that I described above, because I built an add-on, that I pulled from Github. In the latest commit a widget was removed. On my dev environment it still had a widget definition, that was not removed, because of the missing folder in the latest commit. In the end I was not able to upgrade the add-on with the created build, because it contained the orphaned widget definition, but not the needed widget class. The upgrade stopped with an error and the add-on was in an "inconsistent state". Neither the developer nor me knew why there was that orphaned piece of data and it took quite a while to find out what I posted above.

 

[Xon]

There is only one feature that _metadata.json enables that isn't possible otherwise (vs clearing dangling references & importing _output); and that is tracking if the "cache global" flag has been set for a phrase.

Except phrase groups makes the "cache global" flag redundant. Phrase groups consume less memory, and have a lower runtime memory and time-cost.

 

[Jeremy P]

It's possible to mangle the tracked version of templates and phrases, especially when working with many parallel branches spanning multiple release cycles. Granted this can be mitigated somewhat if you can build linearly from a blessed installation which tracks the canonical versions in the database.

That said, I agree it introduces more friction than it's often worth, and likely more than strictly necessary.

 

[Kirby]

Committing _metadata.json causes endless merge conflicts, the format is practically made to cause issues. Plus it causes endless pointless churn which makes git change logs harder to work with

IMO it just isn't needed if you include a few extra CLI commands.

Hmm. but this does not preserve version strings and IDs for templates and phrases which could / will cause issues for end users.

So unless some additional machinery is added to rebuild phrase & template version strings / IDs from VCS tags I somewhat fear not keeping _metadata.json in repos is not a real option.

I do agree though that keeping version strings / IDs within repo files is not a that good idea and we should probably try to avoid this.