add TYPO3 v13 compatibility #26

markijan · 2025-06-18T14:56:50Z

No description provided.

lorenzulrich · 2025-09-19T19:51:16Z

Configuration/JavaScriptModules.php

+    'imports' => [
+        // recursive definition, all *.js files in this folder are import-mapped
+        // trailing slash is required per importmap-specification
+        '@typo3/backend/' => 'EXT:backend/Resources/Public/JavaScript/',


The recursive definition here is very problematic - and I don't think it serves any purporse because the EXT:backend JS loading is taken care of by EXT:backend.
If you have any other extension doing overrides of JavaScripts of EXT:backend (e.g. EXT:container) and this extension is loaded after those, the overrides are overwritten again.

Going to have a look into this PR next week or so, along with the others.

Commenting was a good ping and reminder on that.

Path (recursive) is not that of an issue as it is a chain anyway when looked up, but I tend to have "TYPO3 core overrides" in a dedicated sufolder and more specific on the file, explicitly adding the replaced files instead if simply doing that when a file is dropped into that folder, similar like we do that already for examle here:

https://github.com/web-vision/deepl-base/blob/main/Configuration/JavaScriptModules.php

I don't think I take this PR in that way and splitt it up as I prefere dedicated/logical splitted commits / changes. But I will see next week.

In my situation, when debugging with Xdebug, the following seemed to happen:

The configuration from EXT:backend is resolved and applied --> all files from EXT:backend read

The overrides from EXT:container (single files) are applied

Then the configuration from the v13 branch of EXT:wv_file_cleanup again included all fields from EXT:backend and the overrides from container were overwritten

Going to debug that in core as that is a fancy core behaviour than anyway.

As already mentioned will change that anyway to explicitly add override files explicitly.

IMO it can be just removed as in the v13 branch, no JS from EXT:backend is overridden. Just one JS file included.

We will see.

I recently stramlined the toolchain, the next step would be to check the v12 support first (regarding reported issues) and fix that for current main / 2..x line. after that will be branched out and new major than dropping v11 support and adding v13 support. So that needs to wait anyway a little bit.

sbuerk

@markijan Thanks for providing this pull-request ❤️

General

Some changes are required, because we don't do the
one or other thing and align with TYPO3 Development
or have other preference's doing things.

The pull-request requires a rebase anyway to be based
on the latest changes merged into master meanwhile,
which changes the one or other thing anyway.

Some hints

@markijan In general we use the same contribution and
commit rules like it is used for TYPO3 Core Development,
including taking commit message as first source of truth
and requiring to have proper splitted/composed commits
with suiting commit messages, following a logical order.
And we also uses the commit message prefixes used by TYPO3
Core Development for our extension repositories and like
other extension authors in the community.

Summary

Important

That was more as a information, no action needed to be taken.

I commented only some stuff and I'm going to:

rebase the whole pull-request when the time is ready to deal
with the pull-request (branched out 2.x, ...).
taking care of my comments, reverting things or align things
to our coding guidelines.
testing and verifiying the changes for TYPO3 v12 and v13 in
a dual version support way and making core version related
implementation of classes or view if required not regonizable
by simply code-reviewing the pull-request on github.
taking care of comments by other contributors.

sbuerk · 2026-01-03T23:12:21Z

Classes/Command/CleanupCommand.php

    protected FileRepository $fileRepository;

-    public function injectFileRepository(FileRepository $fileRepository): void
+    public function __construct(FileRepository $fileRepository)


@markijan

We prefer to use inject* method injection in commands to avoid
messing around with the constructor, which is defined by upstream
symfony component and may change any time.

Can you explain why you consideredto switch to constructor injection
here and missing the scalar nullable name argument not passing it down
to the parent call anyway ?

I'm going to revert this when streamlining/updateing the pull-request.

sbuerk · 2026-01-03T23:13:59Z

Classes/Command/CleanupCommand.php

 /**
 * Class CleanupCommand
+ *
+ * @package WebVision\WvFileCleanup\Command


We don't use the @package annotation in our extensions, similar to TYPO3 not using it within the core code. There is no real benefit of maintaining it.

And defining the "sobfolder" as own "package" does not make sense anyway.

Going to revert this when updating the pull-request.

sbuerk · 2026-01-03T23:15:59Z

Classes/Command/CleanupCommand.php

+The symfony commands wv_file_cleanup:cleanup and wv_file_cleanup:emptyrecycler are available.
+
+Example of using the command controllers from CLI context:
+.vendor/bin/typo3 wv_file_cleanup:cleanup --help


Adding this as class PHP docblock does not make sense at all. I'd say
the command helptext needs to be adjusted to add explain this in more
detail.

Additionally, starting (adding) documentation (Documentation/) in
renderable state should be done and explain it in the documentation
along with adding it to the README.md file.

Going to have look on that when updatein the pull-request for the
required changes and rebasing it after extension repository is in
the correct state to merge this (branching out the 2.x branch).

sbuerk · 2026-01-03T23:18:04Z

Classes/Command/EmptyRecyclerCommand.php

    /**
+     * @param InputInterface  $input
+     * @param OutputInterface $output
+     *


Like core we omit phpdocblock if native types are used until additional information are added, which is not the case here. Further we don't align
phpdocblock properties (column) for the argument name - similar to how it's
done within TYPO3 Core Development because we follow the core style making
extensions within our company an align the coding style to it.

Going to revert this when updating the pull-request.

sbuerk · 2026-01-03T23:19:46Z

Classes/Domain/Repository/FileRepository.php

+     * @param Folder      $folder
+     * @param bool        $recursive
+     * @param string|null $fileDenyPattern
+     * @param string|null $pathDenyPattern


Like core we omit phpdocblock if native types are used until additional information are added, which is not the case here. Further we don't align
phpdocblock properties (column) for the argument name - similar to how it's
done within TYPO3 Core Development because we follow the core style making
extensions within our company an align the coding style to it.

Going to revert this when updating the pull-request.

sbuerk · 2026-01-03T23:22:31Z

Resources/Private/Language/locallang_mod_cleanup.xlf

 				<source>Cleanup selected files</source>
 			</trans-unit>
+			<trans-unit id="displayThumbs">
+				<source>Display thumbnails</source>


There is no need to add this here. The label was used from
TYPO3 core and has been moved in TYPO3 v12 already, which
has been missed to adopt.

Meanwhile this has been mitigated with #40.

That means this can be removed here again.

sbuerk · 2026-01-03T23:24:25Z

Resources/Private/Templates/Cleanup/Index.html


-		<f:if condition="{checkboxes.displayThumbs.enabled}">
+        <h3>{f:translate( key:'LLL:EXT:wv_file_cleanup/Resources/Private/Language/locallang_mod_cleanup.xlf:settings')}</h3>
+		<f:if condition="{checkboxes.displayThumbs.enabled} == 222">


Can't grasp this change from reading. Why adding a condition for the enabled state which should be a true/false (0/1) value and not 222 ?

Is this a left-over ?

sbuerk · 2026-01-03T23:25:42Z

composer.json

    "require": {
 		"php": ">=7.4 || >= 8.0",
-        "typo3/cms-core": "^11.5 || ^12.4"
+        "typo3/cms-core": "^12.4 || ^13.4"


We are going to remove this from this pull-request when we have the
state (branched out 2.x) and prepared master branch adding this
constraint.

Good to see that in general "dual TYPO3 version" support has been
regocinized and reflected. +1

sbuerk · 2026-01-03T23:27:43Z

composer.json

+        "typo3/cms-core": "^12.4 || ^13.4"
    },
    "require-dev": {
        "typo3/testing-framework": "^6.16 || ^7.0",


testing-framework and phpunit needs to be updated when switching
to TYPO3 v12/v13 support, which has not be done, mainly because
there were no real tests in the past - which we have now meanwhile,
at least the infrastructure for it.

Also missing are the adoption in the GitHub action workflow files.

Something which would bubble up when pull-request will be rebased
onto synced master, basing it on the latest merged changes.

sbuerk · 2026-01-03T23:29:53Z

Resources/Private/Templates/Cleanup/Index.html

+                        <th nowrap="nowrap" class="col-icon"></th>
+                        <th nowrap="nowrap" class="col-title">{f:translate( key:'LLL:EXT:filelist/Resources/Private/Language/locallang_mod_file_list.xlf:c_name' )}</th>
+                        <th nowrap="nowrap" class="col-path">{f:translate( key:'LLL:EXT:wv_file_cleanup/Resources/Private/Language/locallang_mod_cleanup.xlf:filepath' )}</th>
+                        <th nowrap="nowrap">{f:translate( key:'LLL:EXT:filelist/Resources/Private/Language/locallang_mod_file_list.xlf:c_record_type' )}</th>


We should get the label keys injected into the view and
using the recently added TranslationLabel service [1]
and extending it with these labels.

With new MajorVersion we should also switch to full label
keys like used here and all the other places, for example
the controller.

[1] #40

sbuerk · 2026-01-03T23:49:20Z

Resources/Private/Templates/Cleanup/Index.html

 	  xmlns:fl="http://typo3.org/ns/WebVision/WvFileCleanup/ViewHelpers">

-<f:layout name="Default" />
+<f:layout name="Module" />


What is the reason to change the layout here? To which TYPO3 Changelog entry is this related and required ? TYPO3 v12 module api changes ?

Do we need to make v12/v13 different handling here or would that work for both versions ?

(Something commit messages are the right place to explain things like that).

sbuerk · 2026-01-03T23:51:16Z

Classes/Domain/Repository/FileRepository.php

 use TYPO3\CMS\Core\Resource\ProcessedFile;
 use TYPO3\CMS\Core\SingletonInterface;
 use TYPO3\CMS\Core\Utility\GeneralUtility;
+use TYPO3\CMS\Extbase\Utility\DebuggerUtility;


Why is this import added ? Should not end up in production code. Is this a "debugging" left-over not using xdebug over these tools ?

Markijan Baran added 2 commits June 18, 2025 17:43

add TYPO3 v13 compatibility

a01ee6b

add JS modules config

99f9d20

dhuf mentioned this pull request Sep 3, 2025

TYPO3 13 #37

Open

lorenzulrich reviewed Sep 19, 2025

View reviewed changes

sbuerk requested changes Jan 3, 2026

View reviewed changes

add TYPO3 v13 compatibility #26

Are you sure you want to change the base?

add TYPO3 v13 compatibility #26

Uh oh!

Conversation

markijan commented Jun 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbuerk Sep 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbuerk left a comment

Choose a reason for hiding this comment

General

Some hints

Summary

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

sbuerk Sep 19, 2025 •

edited

Loading