IchHabRecht / filefill

Find and fetch missing local files from different remotes
GNU General Public License v2.0
63 stars 34 forks source link

Undefined array key "tableName" #60

Open nvsx opened 1 year ago

nvsx commented 1 year ago

Screenshot 2022-12-08

When adding a content element to a page, there is a warning message displayed in the backend.

This message only occurs while also using the "gridelementsteam/gridelements" extension together with filefill. (I did not get it while using PHP 7.4.)

Filefill 4.1.1 PHP v8.1

Message is: Core: Error handler (BE): PHP Warning: Undefinded array key "tableName" in /var/www/html/public/typo3conf/ext/filefill/Classes/Hooks/FlexFormToolsHook.php line 24

Sorry, I can not find the reason for it.

IchHabRecht commented 1 year ago

Hi @kaystrobach,

Thank you for your report. I can't reproduce your issue with a "normal" content element. I guess (according to your screenshot) it's triggered by some gridelements code. I had a look in the core class that triggers the hook and according to that class (\TYPO3\CMS\Core\Configuration\FlexForm\FlexFormTools) both fields are absolutely needed in this context. It seems that gridelements provides a wrong context variable ($identifier). Maybe you have time to validate this guess from reading the code. I think if there is an issue within gridelements that should be handled in there.

kaystrobach commented 1 year ago

It's related to a gridelement here as well. maybe @bunnyfield can take a look, will ask him. Thanks a lot for the fast feedback.

kaystrobach commented 1 year ago

I talked to @bunnyfield right now. He said that the consumer has to verify the existence of array keys (also for PHP8), so there won't be a fix in gridelements for exactly this problem.

How to proceed here? In the worst case I'm able to use composer patches for my use case.

IchHabRecht commented 1 year ago

The core itself throws exceptions when the mentioned array keys are not properly provided. How can @Bunnyfield say that the gridelements code work correct here? While filefill can fix the notice it's not a valid fix in my point of view. As I don't have any insides into gridelements I can't say anything about the missing array keys here.

kaystrobach commented 1 year ago

Ok, so I will go with our patch for now, feel free to close the ticket / PR. I informed @bunnyfield. Thanks a lot for your time.

kaystrobach commented 1 year ago

He might also check GE lateron.

Bunnyfield commented 1 year ago

The point is not, if Gridelements is correct in this case. Array keys have to be checked anyway to make sure PHP 8+ does not throw exceptions. So there might be a fix in Gridelements, since we've got an issue reported there too - have to investigate though - still this should be fixed in filefill too.

Bunnyfield commented 1 year ago

Just checked the core function getDataStructureIdentifier to see what is expected there:

        // Hook to inject an own logic to point to a data structure elsewhere.
        // A hook has to implement method getDataStructureIdentifierPreProcess() to be called here.
        // All hooks are called in a row, each MUST return an array, and the FIRST one that
        // returns a non-empty array is used as final identifier.
        // It is important to restrict hooks as much as possible to give other hooks a chance to kick in.
        // The returned identifier is later given to parseFlexFormDataStructureByIdentifier() and a hook in there MUST
        // be used to handle this identifier again.
        // Warning: If adding source record details like the uid or pid here, this may turn out to be fragile.
        // Be sure to test scenarios like workspaces and data handler copy/move well, additionally, this may
        // break in between different core versions.
        // It is probably a good idea to return at least something like [ 'type' => 'myExtension', ... ], see
        // the core internal 'tca' and 'record' return values below

That's exactly what gridelements does.

It either returns a full identifier array containing the given datastructure information with tableName and fieldName or it returns an array ['type' => 'gridelements-dummy'] when there is no given DataStructure at all and if the CType is not gridelements_pi1 it will return an empty array.

So there is nothing to change from the Gridelements side.

sypets commented 1 year ago

Had the same error message on updating the reference index.

It is of course not acceptable to not be able to update the reference index.

I don't know if it is a gridelements problem or not but I think it is not unreasonable to check for existing array elements (e.g. with null coalesce). There have been numerous patches both in the core and in extensions which do exactly that. Unless TYPO3 decides to move away from using arbitrary arrays to well defined classes, it is hard to tell if a specific entry will or should always exists in the passed array.

(Ideally, we apply a fix in both extensions).

versions

I am using

code:

filefill/Classes/Hooks/FlexFormToolsHook.php

24: if ($identifier['tableName'] !== 'sys_file_storage'

Have upvoted the PR #66, will use it as patch for now.

maikschneider commented 1 year ago

Same problem here, #66 should be merged.

nabossha commented 11 months ago

exact same problem #66 fixes it for me on 11.5.x. with PHP 8.0 is there any chance of merging this?

mtness commented 2 months ago

Hio there, it would be very nice to merge this finally!