Fix attachments renaming

[trasher]

Q	A
Bug fix?	yes
New feature?	no
BC breaks?	no
Deprecations?	no
Tests pass?	yes
Fixed tickets	#9667 #9600

[cedric-anne]

Unless I am wrong, if filename does not have a _X suffix, filename will not be modified, and so it may result in an infinite loop.

[trasher]

@cedric-anne this is a private method, so it cannot be tested directly. I'm not going to write unit tests for that; it would take too much time.

[BorisNovg]

Fix 89104ec (#9672) solves the problem.

I tested using the message Message16335125161734170604.zip.

In addition, I created a message with three different attachments, but having the same name without an extension.
Message-test_3_different_attachments_with_the_same_filename_without_extension.zip

All three files have been saved, their names are:
1234567890
1234567890_1
1234567890_2

Ok!

Also, I checked a letter in which two screenshots are attached inside the letter, different but with the same file name.
The file name is =?US-ASCII?B?0LjQt9C+0LHRgNCw0LbQtdC90LjQtS5wbmc=?=, which means изображение.png (screenshot.png in Russian).

As shown in the screenshot, 2 images have been successfully inserted into the message text.
Ok!

All five documents received by mail:

In addition, I sent copies of the last 538 messages from the my production system to the mail receiver. No errors were found.

Maybe you can suggest additional testing methods?

[BorisNovg]

So far, I have only checked messages with attached files with file names without an extension. I am correcting and adding a check for file names with an extension.

Added a message check that contains two images (screenshots) with different contents, but with the same file name with the extension (image.png). In addition, the files are not just attached, but inserted as links in the html part of the message (tags <img style="width: 823px;" src="cid:Hi34@LVGiLJxJ.mevFqq1U "> and <img style="width: 823px;" src="cid:fnIp@hhaD9S5z.0pukDqWx ">).

Sample message
Message-test_2_different_inline_attachments_with_the_same_filename_with_extension.zip

Two files with the same file name image.png are saved with different names image.png and image_1.png, the html part of the message is displayed correctly. The file name was changed by adding the suffix "_1" to the base part of the name.
Ok!

[BorisNovg]

Maybe you can suggest additional testing methods?

[BorisNovg]

You have added a variant of a test message with three IDENTICAL attachments. It seems to me that in life there may be more often an option with several attachments having the same file name, but with DIFFERENT contents.

I propose a change:

glpi/tests/emails-tests/28-multiple-attachments-no-extension.eml

Line 52 in 71a79b1

MTIzNDU2Nzg5MA==

MTIzNDU2Nzg5MDEyMzQ1Njc4OTA=

glpi/tests/emails-tests/28-multiple-attachments-no-extension.eml

Line 58 in 71a79b1

MTIzNDU2Nzg5MA==

MTIzNDU2Nzg5MDEyMzQ1Njc4OTAxMjM0NTY3ODkw

In your version, the second and third attachments will be discarded with the message "glpiphplog.ERROR: Document_Item::prepareInputForAdd() in /usr/share/glpi/inc/document_item.class.php line 125 Duplicated document item relation"

In my version, all three attached files will be saved, my check is more complete.

[trasher]

Hello @BorisNovg , Thank you very much for your tests and feedbacks!!

I'm trying to add your tests cases in our suite, I do not have any extra case to suggest.

[BorisNovg]

One more small remark. I'm not sure that this is a matter of principle, but I want to note that until now (in release versions 9.5.5 and 9.5.6) when renaming a file a suffix was added starting with "_2". In the proposed version a suffix is added starting with "_1". Probably the old renaming method was more correct and cosmetically more beautiful. Perhaps this will be important for some plugins.

Here is a screenshot from the previous version:

Here is a screenshot from the version 89104ec

Maybe make such a change as shown below?

glpi/inc/mailcollector.class.php

Line 1597 in 89104ec

$basename .= '_1';

$basename .= '_2';

I tested it on two of my messages. There were no errors.

[trasher]

No problem for me to switch back to _2; that does not matter from the code point of view.

[BorisNovg]

Sorry to barge in. Perhaps this line should be deleted.

glpi/tests/emails-tests/28-multiple-attachments-no-extension.eml

Line 9 in 0e6fda7

Content-Transfer-Encoding: base64

It was probably accidentally added when cleaning the headers (@trasher Cleanup headers 811a436). In my original example, there is no such string.

[BorisNovg]

I noticed that during the control testing these tests were failed
"Test on PHP 8.0 using mariadb:10.6" - "IMAP test"
and
"Test on PHP 8.0 using mysql:8.0" - "IMAP tests"

with log messages:
*** Uncaught Exception Laminas\Mail\Exception\InvalidArgumentException: The input is not a valid email address. Use the basic format local-part@hostname in /var/glpi/vendor/laminas/laminas-mail/src/Address.php at line 78

*** Uncaught Exception Laminas\Mail\Storage\Exception\InvalidArgumentException: Header with Name date or date not found in /var/glpi/vendor/laminas/laminas-mail/src/Storage/Part.php at line 307

As a solution, you are trying to change the test email glpi/tests/emails-tests/28-multiple-attachments-no-extension.eml by focusing on these messages in the log (you have changed the email and data fields), assuming that it is the text in the test email that creates these errors.

However, these are most likely erroneous actions, since exactly the same error messages are present in absolutely all successful tests, for example, a month ago:
https://github.com/glpi-project/glpi/pull/9529/checks?check_run_id=3566315938#step:14:22
https://github.com/glpi-project/glpi/pull/9529/checks?check_run_id=3566315938#step:14:23

The presence of these messages in the log caused a warning, but not an error when completing the IMAP test:
https://github.com/glpi-project/glpi/pull/9529/checks?check_run_id=3566315938#step:14:19
Success (1 test, 10/10 methods, 0 void method, 0 skipped method, 276 assertions)!

IMHO the test completion error is related to adding a file glpi/tests/emails-tests/28-multiple-attachments-no-extension.eml, however, the reason for the error is not related to the log messages about the email and date fields, and this reason is still unknown to us.

Perhaps the reason for the error was indicated in the following lines (they are incomprehensible to me):

==> -Expected
==> +Actual
==> @@ -1 +1 @@
==> -int(0)
==> +int(2) in /var/glpi/vendor/atoum/atoum/classes/asserter.php at line 131"
Error: Process completed with exit code 1.

[trasher]

with log messages: *** Uncaught Exception Laminas\Mail\Exception\InvalidArgumentException: The input is not a valid email address. Use the basic format local-part@hostname in /var/glpi/vendor/laminas/laminas-mail/src/Address.php at line 78

*** Uncaught Exception Laminas\Mail\Storage\Exception\InvalidArgumentException: Header with Name date or date not found in /var/glpi/vendor/laminas/laminas-mail/src/Storage/Part.php at line 307

Indeed, those ones are "expected", good catch.

Perhaps the reason for the error was indicated in the following lines (they are incomprehensible to me):

==> -Expected
==> +Actual
==> @@ -1 +1 @@
==> -int(0)
==> +int(2) in /var/glpi/vendor/atoum/atoum/classes/asserter.php at line 131"
Error: Process completed with exit code 1.

Yes, I saw that one and spent time on it; but I do not understand what the problem is :/

[BorisNovg]

Maybe this script output has useful information?

Run .github/actions/init_initialize-imap-fixtures.sh
Initialize email fixtures

GLPI dataset version 4.7 already loaded

.......U..                                                   [10/10]

If I understood this script output correctly, the failure occurred precisely in the eighth test (out of 10).

[trasher]

If I understood this script output correctly, the failure occurred precisely in the eighth test (out of 10).

Yes, that's it... But this test is the one that imports all emails from the inbox; and it does many assertions... The test framework should not fail in any internal class; but it does (maybe a kind of bug on their side, difficult to know).

[trasher]

Great, many thanks @cedric-anne for tests debug!