i seriously doubt anyone would ever need this feature:)

Lets not register those to the classloader please

CommentFileSizeAuthor
#5 drupal-classloader_themes-2070581-5.patch1.62 KBParisLiakos
#2 drupal-classloader_themes-2070581.patch503 bytesParisLiakos

Comments

dawehner’s picture

dawehner
Primary language German
commented

Related issue #2002606: Allow themes to provide services.yml

ParisLiakos’s picture

ParisLiakos commented
Status: Active » Needs review
StatusFileSize
new drupal-classloader_themes-2070581.patch503 bytes

i think keeping the drupal_get_filename() call though is good, so lets keep call system_register

lets see if something breaks

Status: Needs review » Needs work

The last submitted patch, drupal-classloader_themes-2070581.patch, failed testing.

ParisLiakos’s picture

ParisLiakos commented

so we actually have a test for this;)
Drupal\system\Tests\Theme\ThemeTest->testClassLoading()

ParisLiakos’s picture

ParisLiakos commented
Status: Needs work » Needs review
StatusFileSize
new drupal-classloader_themes-2070581-5.patch1.62 KB

Status: Needs review » Needs work

The last submitted patch, drupal-classloader_themes-2070581-5.patch, failed testing.

berdir’s picture

berdir
Primary language German
Location Switzerland
commented
Status: Needs work » Needs review

#5: drupal-classloader_themes-2070581-5.patch queued for re-testing.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

The test was deliberately added in #1538478: Register lib/ directories of themes as PSR-0 roots for Drupal\$theme namespace, and back then it was decided that themes *should* be able to autoload classes if they need them. What benefits do we get from removing this?

iamEAP’s picture

iamEAP commented

Relevant discussion from #8:

Crell:

The theme system is scheduled to be gutted by a series of sprints starting this weekend. We may want to wait until then to spend more time on this. Certainly IF a theme is going to include classes, they should be PSR-0-loaded, no question. Whether or not it will, we don't know yet.

sun:

I don't think this needs much discussion. Themes won't be "PHP-less" anytime soon, so it would be very strange to not support class autoloading for themes for no apparent reason.

xjm’s picture

xjm
she/her
Primary language English
commented
Issue summary: View changes
Status: Needs review » Needs work
Issue tags: +Needs reroll

The patch here couldn't be automatically rerolled for #2247991: [May 27] Move all module code from …/lib/Drupal/… to …/src/… for PSR-4 (the only one among best-guess patches that apply), but it sounds like maybe this issue is a wontfix anyway?

xjm’s picture

xjm
she/her
Primary language English
commented
Issue tags: -Needs reroll

Changed my mind; not tagging based on #8 and #9.

markhalliwell’s picture

markhalliwell
Primary language English
commented
Status: Needs work » Closed (won't fix)

Sigh... I really cannot wrap my head around why themes are consistently targeted as "lower class". This mentality has to stop. Seriously.

They have PHP code and I don't see that going away anytime soon. Until themes don't have a single line of PHP code, something like this should never be added.

Limiting a theme's ability to utilize native and fundamental PHP syntax just because they haven't been using them is extremely presumptuous.

The primary reason themes haven't used namespaces in the past is because 7.x didn't support them out-of-the-box and would require themes to add a module dependency (which is very difficult to support in a theme).

Thus, namespaces really hasn't be an "official thing" until 8.x and I, for one, am rather excited to start using them to finally organize and OO some of the Bootstrap base theme's code.

So, as a themer and maintainer of one of the top d.o themes, I'm marking this as "won't fix".