Prevent directory modifications when iterating

[caco3]

This PR solves the failing updates due to issues on removing the old files and copying the new files.
See #158 for details.
And it also solves the issue some systems have when modifying a directory while iterating through it, see #509 for details.

TL;DR

On some servers (notable FreeBSD 12.4 on hostspoint.ch), the updater tries to rmdir() non-empty folders. This is not supported on PHP. The updater already has a function to recursively delete (recursiveDelete()), how ever it is not used consistently.

Additionally, the iterator gets gets messed up in the move step when modifying the directories. Thus we copy() instead of rename() and remove the whole folders/files afterwards in a separate iteration.

Scroll down for the fix.

Issue

Updater fails to delete old files:

updater log on first deletion failure:

2023-10-23T00:06:59+0200 xGP6zzWOT0 [info] startStep("9")
2023-10-23T00:06:59+0200 xGP6zzWOT0 [info] deleteOldFiles()
2023-10-23T00:07:25+0200 xGP6zzWOT0 [info] config sample exists
2023-10-23T00:07:25+0200 xGP6zzWOT0 [info] themes README exists
2023-10-23T00:07:25+0200 xGP6zzWOT0 [error] POST request failed with other exception
2023-10-23T00:07:25+0200 xGP6zzWOT0 [error] Exception: Exception
Message: Could not rmdir: /home/xxx/www/nextcloud/updater/../lib/l10n
Code:0
Trace:
#0 /home/xxx/www/nextcloud/updater/index.php(1388): Updater->deleteOldFiles()
#1 {main}
File:/home/xxx/www/nextcloud/updater/index.php
Line:918

The general issue seems to be that rmdir only is able to delete empty folders.
The first time the issue occurs for me is on deleting the lib/l10n folder.
Looking on the folder, it clearly is not empty at that time:

> ls
af.json    az.js      br.js      el.js      es_AR.json es_EC.json es_PR.json fo.js      gd.json    hy.js      id.json    km.js      lb.js      mk.json    nl.json    pt_BR.js   si.js      th.js      vi.json
an.js      az.json    bs.js      el.json    es_CL.js   es_HN.json es_UY.json fo.json    he.js      hy.json    is.json    kn.js      lb.json    ms_MY.js   oc.js      pt_BR.json sk.js      th.json    zh_CN.js
ar.js      be.js      cs.json    eo.js      es_CL.json es_MX.json et_EE.json fr.js      hr.json    ia.js      it.js      kn.json    lo.json    nb.json    pl.js      pt_PT.js   sq.js      tk.json    zh_TW.js
ast.js     bg.json    da.js      eo.json    es_CO.js   es_NI.json eu.json    fr.json    hu.js      ia.json    ja.js      ko.js      lt_LT.json ne.json    pl.json    pt_PT.json sq.json    ur_PK.js
ast.json   bn_BD.js   de_DE.json es_419.js  es_DO.js   es_PE.js   fi.json    gd.js      hu.json    id.js      ka_GE.js   ko.json    lv.json    nl.js      ps.js      sc.json    ta.json    ur_PK.json

Note that it still contains 93 files. Odly, originally it contained 200 files, so some successed to get deleted...

Fix

• do not loop directly through the iterator but generate a file list and iterate through that one (on BSD it seems that the iterator gets messed up when the directory gets modified)
• Validate return codes of rmdir() and unlink().
• make log messages consistent
• minor fixes

Using that fix, I am successful to update Nextcloud 27.0.0 to the latest (currently 27.1.2).

[Altahrim]

Thank you for the work and new clean PR :)

With your previous PR, I noticed the the cs-fixer wasn't using the current Nextcloud guidelines (but our code was), so I updated them. I think you can skip the formatting commit.

[caco3]

With your previous PR, I noticed the the cs-fixer wasn't using the current Nextcloud guidelines (but our code was), so I updated them. I think you can skip the formatting commit.

I didn't do the formating commit because of a coding guideline, but because the files where inconsistent (and I am used to have separating whitespaces between operators).

But there is still one failing check and I do not understand what I am supposed to do with it.

[Altahrim]

I tried to generate the updater.phar myself 🤞

[caco3]

Thanks. And it seems to pass now.

But you misst to sign your commit. -> I am glad I am not the only one doing that mistake :)

[Altahrim]

Seems good.
Can you rebase to integrate to my commit? I think you can drop your latest commit.

[Altahrim]

But you misst to sign your commit. -> I am glad I am not the only one doing that mistake :)

Yes, I only did a fixup :)
I am usually use git rebase origin/master -i --autosquash --signoff to do that.

[marcelstoer]

@Altahrim thanks for merging

How will we find out which NC release is going to contain this fix? Through the NC release notes?

[blizzz]

Master will become NC 28, and this will be shipped with Beta 2, and thus become effective with Beta 3.

@Altahrim what do you think about backport to 27 and 26? I'd fancy having #507 solved in that case first.

[caco3]

I also would suggest to have a look on all other usages of RecursiveDirectoryIterator() in NC.
Since the root cause is not (directly) caused by BSD but could occur on any system.

[Altahrim]

@blizzz Yes, I think it would be great to backport and fix #507 too.
I'll try to take time for this.

[github-actions]

Hello there,
Thank you so much for taking the time and effort to create a pull request to our Nextcloud project.

We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process.

Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6

Thank you for contributing to Nextcloud and we hope to hear from you soon!