search

andersundsehr / aus_driver_amazon_s3

Provides a TYPO3 FAL driver for the Amazon Web Service S3
GNU Lesser General Public License v3.0
21 stars 40 forks source link

[BUGFIX] Register the driver via $GLOBALS #41

Closed vertexvaar closed 5 years ago

vertexvaar commented 6 years ago

TYPO3 extensions must not use the DriverRegistry to register their driver implementations. See the TYPO3 coredev channel message https://typo3.slack.com/archives/C03AM9R17/p1538658116000100 for more information.

Lagerregal commented 6 years ago

Thanks for your patch! But I don't know if this is a good idea... I think we are using this method because an other extension maybe initialized the DriverRegistry before the S3 driver. So our driver is not used, without using the DriverRegistry... Of course this issue exists in the other direction, if any other extension wants to change something afterwards in the $GLOBALS ...

--> So every extension has to use the DriverRegistry or every extension has to use the $GLOBALS array. Mix this up is not possible. I think this should be clarified and fixed by the core team. What dou you think?

A workaround for the S3 driver could be: Check if the DriverRegistry is already initialized before by other extensions. If not just use the $GLOBALS array.

ghost commented 6 years ago

Hi Lagerregal. According to the answer in the Typo3 coredev channel :

Benjamin Franzke say :

DriverRegistry::registerDriverClass() is prohibited there.

If I would have known that prior the v9 release I would have made a patch for deprecating that (DriverRegistry::registerDriverClass() in ext_localconf.php), similar to how we forbid CacheManager::createCache during ext_localconf.php as that exposes a similar problem.

Lagerregal commented 6 years ago

Ok, maybe we should add a version check to keep backward compatibility for TYPO3 versions below version 9?

vertexvaar commented 6 years ago

@Lagerregal the pull request will make this extension TYPO3 v11 (when registerDriverClass will be removed) compatible while retaining full backwards compatibility down to TYPO3 v6 when FAL was introduced regarding the driver registration. And anyone that uses the DriverRegistry should be informed about the breaking nature it has and that it must be changed sooner or later anyways. Registering the driver via $GLOBALS is and has always been the right way.

I think we are using this method because an other extension maybe initialized the DriverRegistry before the S3 driver.

Which is a problem @stephane-wecker had when he installed both our extensions (this S3 Fal Driver and my SFTP driver). This driver uses the DriverRegistry which prohibits the SFTP Driver from showing up. When everybody uses $GLOBALS nobody will block any other extension and nobody will be forced to manually register the driver in their LocalConfiguration.php or use the DriverRegistry which was never meant for driver registration/direct usage.

I think this should be clarified and fixed by the core team. What dou you think?

As linked and also quoted by @stephane-wecker the message from the core dev team is very clear about that. We ~should~ must not use the DriverRegistry

I don't think there is any reason left not to merge it and i hope we (and the core team) could convince your :wink:

Lagerregal commented 6 years ago

I'm a little afraid of merging this because it can break existing instances. So I would introduce this change for this extension with TYPO3 version 9 and maybe add a documented option to using DriverRegistry as deprecated fallback.

vertexvaar commented 6 years ago

it can break existing instances

Well it will not as long as there is no other extension installed which uses DriverRegistry in its ext_localconf.php and is loaded before this driver. And if, that should be fixed in the next extensions. Anyway there is also an easy workaround for that.