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.

 

[Xon]

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

As long as the versionId never goes backwards; XF's outdated template detection works as expected and phrases work too.

It is part of the reason I set a unix-like timestamp as the version-id for my add-ons instead a integer encoded copy of the version string. I've got a CLI clamp-versions command which is part of my built script, and that ensure version ids aren't newer than the add-on version I'm building.

 

[Jeremy P]

It’s not that they won’t be updated, it’s that the update could be attributed to the wrong version ID at the time they’re imported into the machine the add-on is built with, which only later impacts things like template customization tracking for admins.

I think it’s far less likely to happen in practice if you're not merging across different release branches though, which I imagine is the case for most (but not all) add-on developers.

 

[Kirby]

As long as the versionId never goes backwards; XF's outdated template detection works as expected and phrases work too.

Unfortunately it does not work for me or I am doing smth. wrong.

Steps to reproduce

1. Create initial Addon
   

   php cmd.php xf-addon:create

2. Create template kirby_mdversiontest_tpl1with content
   

   This template was created in version 1.0.0 and not touched afterwards

   php cmd.php tck-devtools:clamp-versions Kirby/VersionMetaDataTest
   php cmd.php xf-addon:build-release

3. Copy current state to simulate push to remote repo and uninstall afterwards
   

   cp -pr src/addons/Kirby/VersionMetaDataTest /tmp
   php cmd.php xf-addon:uninstall Kirby/VersionMetaDataTest
   rm -rf src/addons/Kirby/VersionMetaDataTest

4. Simulate work continuation by another developer / another machine that does not have 1.0.0 installed
   

   cp -pr /tmp/VersionMetaDataTest src/Kirby
   php cmd.php xf-addon:install Kirby/VersionMetaDataTest
   php cmd.php xf-addon:bump-version --version-id=1000170 Kirby/VersionMetaDataTest

5. Create template kirby_mdversiontest_tpl2with content
   

   This template was created in version 1.0.1

6. php cmd.php tck-devtools:clamp-versions Kirby/VersionMetaDataTest
   php cmd.php xf-addon:build-release Kirby/VersionMetaDataTest

7. Simulate build on fresh instance from VCS not containing _metadata.json
   

   php cmd.php xf-addon:uninstall Kirby/VersionMetaDataTest
   rm src/addons/Kirby/VersionMetaDataTest/_output/templates/_metadata.json
   php cmd.php xf-addon:install Kirby/VersionMetaDataTest
   php cmd.php xf-addon:build-release Kirby/VersionMetaDataTest

8. Install version 1.0.0 from ZIP on a different non-development instance
9. Modify template kirby_mdversiontest_tpl1 on that instance
10. Upgrade to version 1.0.1 from ZIP from step 7

Expected result
Template kirby_mdversiontest_tpl1 is Version 1.0.0 and not shown as outdated on the target system

Actual result
Template kirby_mdversiontest_tpl1 is Version 1.0.1 and shown as outdated on the target system

I think not having _metatdata.json in VCS only works if all development work (touching templates or phrases) is only ever done on the same instance without fresh install / import from VCS.

But this is not the case if there are multiple developers working on the code and / or when building the release via CI/CD from VCS.