search

fnagel / generic-gallery

TYPO3 extension: Generic Gallery - One gallery to rule them all!
https://extensions.typo3.org/extension/generic_gallery/
GNU General Public License v3.0
7 stars 12 forks source link

Autoloading for classes #10

Closed liayn closed 5 years ago

liayn commented 7 years ago

Hi!

Is there a reason you a) use TYPO3 as vendor in the namespace (which is highly discouraged b) do not use PSR-4 autoloading?

Would you mind if I push a fix for this?

liayn commented 7 years ago

Forget my point b) again. Overlooked the definition. The vendor name should still be changed though.

But why do you need this? require_once(ExtensionManagementUtility::extPath('generic_gallery').'Classes/Domain/Model/Dto/EmConfiguration.php');

liayn commented 7 years ago

Another question: why do you exclude PHP 7.1?

fnagel commented 7 years ago

Hey there!

use TYPO3 as vendor in the namespace

Afaik only TYPO3\CMS is reserved but I know the current NS is not optimal. I plan to move ALL my extensions to a custom vendor NS but still undecided which one ;-)

But why do you need this? require_once(ExtensionManagementUtility::extPath('generic_gallery').'Classes/Domain/Model/Dto/EmConfiguration.php');

Changed this lately (see 9abd1fffb7676069d209e1258f83b34408731cbb) as GeneralUtility::requireOnce has been deprecated. To be honest, I took the idea from this straight from EXT:news and it seems it's changed there too.

why do you exclude PHP 7.1?

Only because it's untested and I don't like to say its ready if I'm not sure about it. Are you able to confirm everything works fine?

liayn commented 7 years ago

Vendor= FNAGEL?

Changed this lately (see 9abd1ff) as GeneralUtility::requireOnce has been deprecated. To be honest, I took the idea from this straight from EXT:news and it seems it's changed there too.

Why do you need to require the file at all? We have autoloading in place, so it will find the file anyways.

I didn't test with PHP7.1 but there is really low chance that something really breaks with it, since there are no real breaking changes.

fnagel commented 7 years ago

Vendor= FNAGEL?

or FNA (which might be taken already) or FelixNagel or FelixNagelCom (upper camel case is better, right?). Anyway, I need some time to change this for all my extensions...

Why do you need to require the file at all?

Good question, actually. Doesn't seem to make sense. As said I took this straight from EXT:news: https://github.com/georgringer/news/blob/master/Classes/Utility/EmConfiguration.php#L29

Anyway, seems worth trying to remove it.

I didn't test with PHP7.1 but there is really low chance that something really breaks with it, since there are no real breaking changes.

I don't have any experience with 7.1 yet, so I can't speak from experience but I guess you're right.

liayn commented 7 years ago

FNAGEL or FNA

It simply does not matter. And I doubt that even if it is taken that the extensions will have the same name too. So that's unique enough.

require once in ext:news

I would say this is legacy and a leftover. This thing is in there since at least 2015. We can ask Georg for details.

PHP 7.1

We run live sites on 7.1 without any issues. (Except if you use ext:yag, which really is not 7.1 compatible)

fnagel commented 5 years ago

Closed with b177dcde057e93f84e5d73ab788cf3f0f801e0a3