TASK: Create a new package for the Media Browser #159

dfeyer · 2015-10-26T22:52:32Z

This change create a new package Neos.MediaBrowser to distribute the Neos Media Browser module, so this module is not part of the core.

Resolves: #1130, NEOS-1683

dfeyer · 2015-10-26T22:53:27Z

Next step should be to remove the templates and controller from the Media package to have everything in this package.

dfeyer · 2015-10-26T22:57:12Z

Basically the Media Package is working correctly.

The only problem come from the ImageEditor when selecting an image when we try to open the media browser from the inspector Neos throw an exception regarding permissions (The security context contained no tokens which could be authenticated.), no clue why currently, but it's too late for today ... let's see tomorrow

aertmann · 2015-10-27T07:43:47Z

Before anyone merges this we should consider creating a new branch for bigger tasks like this. The prioritizers discussed creating branches for each project/larger change. This will allow us to much better have simultaneous development and only allow finished features into master and not half finished ones. Still need to write a proposal for it though.

dfeyer · 2015-10-27T08:17:47Z

@aertmann I agree with creating branch for those kind of changes, and don't merge this one ;) it's a WIP currently, playground status currently

dfeyer · 2015-10-29T11:01:58Z

Now this PR depends on the Flow PR:
neos/flow-development-collection#114

Now the access to the media browser from the inspector work (policy wise), some CSS/JS loading issue, that need to be solved but at least the security parts work ;)

dfeyer · 2015-11-17T10:42:43Z

Rebase on top of master, and now depend on neos/flow-development-collection#130

kitsunet · 2016-01-26T13:15:17Z

@dfeyer ping if you want someone to have a look and go further with it.

Sebobo · 2016-11-21T09:21:01Z

I'll try to continue this now at the sprint.

Sebobo · 2016-11-23T15:53:02Z

@dfeyer @kitsunet @nezaniel @aertmann the PR is now rebased onto master. All additional adaptions to master from @nezaniel and me are in the second commit.

johannessteu

Tested locally - looks good. Didn't checked all the code

dfeyer · 2016-11-23T23:19:50Z

After installing I have to adapt ContainerViewHelper to not use the Context::getPartyByType but the PartyService. But I think it's not related to this change

I think the Thumbnail controller should also move the the new Neos.Media.Browser package, basically the Neos.Media package should only contains logic and no controller, not routes

I continue testing tomorror

This change extract the MediaBrowser from the Neos core package and create an new package name Neos.Media.Browser. Resolves: NEOS-1683

Sebobo · 2016-11-24T09:09:23Z

@dfeyer @johannessteu I rebased and that should fix the issue.
We had a discussion at the sprint regarding the thumbnail controller where it should be as it's not only media browser related. I would prefer to do that after the name change because we have to hurry here a bit.
If this change now works without issues we should really merge it and then do the rest afterwards and think about where to put the thumbnail stuff.

johannessteu · 2016-11-24T09:11:40Z

Tested it again, still working!

gerhard-boden

When I try to replace a file I get: #1216919280: You are not allowed to perform this action.

Stacktrace:
https://rawgit.com/gerhard-boden/e8383352582b374615bf08ffe2968590/raw/3faf8ea361164fe102a5e020978abffa22085e06/%25231216919280:%2520You%2520are%2520not%2520allowed%2520to%2520perform%2520this%2520action.html

Sebobo · 2016-11-24T11:28:52Z

There is a problem when replacing some images in the demo site but this is also in master.
It then breaks when trying to delete a resource.

@gerhard-boden please check if it works if you logout and login.

dfeyer · 2016-11-24T11:35:26Z

@Sebobo So OK merge this one it work basically and we can discuss the other small point after the namespace hell ;) and have a safe namespace change party

aertmann · 2016-11-28T17:21:28Z

TYPO3.Neos/Resources/Private/Translations/en/Modules.xlf

+			</trans-unit>
+			<trans-unit id="media.replace.previewFile" xml:space="preserve">
+				<source>Preview current file</source>
+			</trans-unit>
 		</body>
 	</file>
 </xliff>


we should move these labels to the media browser package

aertmann · 2016-11-28T17:23:46Z

TYPO3.Neos/Resources/Private/Partials/Module/Shared/DocumentBreadcrumb.html

+        {documentNode.label}
+    </f:for>
+</neos:backend.documentBreadcrumbPath>


this adds undesired whitespace between the labels, which is why it was formatted without new lines

Correct, didn't want to commit that.

aertmann · 2016-11-28T17:24:52Z

TYPO3.Neos/Configuration/Settings.yaml

-        maximumHeight: 250
-      'TYPO3.Neos:Preview':
-        maximumWidth: 1000
-        maximumHeight: 1000


changing these presets is wrong, they belong to Neos and not the media package as they're used throughout the neos backend and not just for the media browser..

also changing them is a breaking change, which is better to avoid

Would love a discussion about this, because it's currently confusing how and where this is used.

aertmann · 2016-11-28T17:26:13Z

TYPO3.Neos/Configuration/Settings.yaml

-                pattern: 'ControllerObjectName'
-                patternOptions:
-                  controllerObjectNamePattern: 'TYPO3\Media\Controller\.*'
+                  controllerObjectNamePattern: 'TYPO3\Neos\Controller\.*|TYPO3\Neos\Service\.*|TYPO3\Media\Controller\.*'


this is a really bad regression reverting the newly formatted requestPatterns into the old format

Ah this was a mistake when rebasing I guess :/

aertmann · 2016-11-28T17:26:43Z

TYPO3.Neos/Configuration/Routes.yaml

+    'NeosMediaBrowserSubroutes':
+      package: 'Neos.Media.Browser'
+      variables:
+        'defaultUriSuffix': '.html'


instead we should use the routes by settings feature and put these in the media browser package

aertmann · 2016-11-28T17:28:26Z

Neos.Media.Browser/Configuration/Views.yaml

+      'Neos.Media.Browser': 'resource://Neos.Media.Browser/Private/Partials'
+
+    templateRootPathPattern: 'resource://Neos.Media.Browser/Private/Templates'
+    templatePathAndFilenamePattern: '@templateRoot/@subpackage/Module/Management/Asset/@action.@format'


the idea was to get rid of this entirely, we don't want any media templates nor labels in Neos

Needs to be cleaned up, when we achieve that we only have one layout and template.

aertmann · 2016-11-28T17:29:32Z

Neos.Media.Browser/Configuration/Settings.yaml

+        - 'resource://TYPO3.Neos/Public/Library/bootstrap-components.js'
+        - 'resource://Neos.Media.Browser/Public/JavaScript/Modules/media-browser.js'
+      styles:
+        - 'resource://TYPO3.Neos/Public/Styles/Neos.css'


we should probably get rid of this as we don't need it to be extensible anymore

Still needed for the iframe version when opening the browser via the inspector. But as I said in my comment to this PR, this needs an improvement.

aertmann · 2016-11-28T17:31:01Z

Neos.Media.Browser/Configuration/Policy.yaml

+
+      -
+        privilegeTarget: 'Neos.Media.Browser:Backend.Module.Media.ManageAssetCollections'
+        permission: GRANT


why not combine these two privileges?

I think this was intentional that not everybody can change the collections.

aertmann · 2016-11-28T17:32:19Z

Neos.Media.Browser/Classes/Controller/Module/Management/AssetController.php

        }

+        // FIXME: Resources are not deleted, because we cannot be sure that the resource isn't used anywhere else.
+        $this->assetRepository->remove($asset);
+        $this->addFlashMessage(sprintf('Asset "%s" has been deleted.', $asset->getLabel()), null, null, array(), 1412375050);
        $this->redirect('index');
    }



we don't need this class do we? everything should fit in one controller now right?

I didn't understand why the controllers are like they are, so a discussion with the involved people would be great.

aertmann · 2016-11-28T17:32:51Z

Neos.Media.Browser/composer.json

+{
+    "name": "neos/media-browser",
+    "type": "typo3-flow-package",
+    "description": "Add description here",


This module allows managing of media assets including pictures, videos, audio and documents.

aertmann · 2016-11-28T17:35:26Z

Hey @Sebobo, @dfeyer, @johannessteu and @gerhard-boden thanks a lot for pushing this one! Unfortunately found some regressions and some things that should be done as well. Anyone up for a round two?

Sebobo · 2016-11-29T11:16:05Z

Hi @aertmann, I would join in again as I now know the code very well after this monster-rebase and adaption ;)

There are big two things which in my opinion have to be cleaned up.
One is that we currently need different layouts when opening the media browser via the backend module and when opening it via the inspector. This is really smelly.
The other thing is the whole thing about thumbnail generation. Where to do it, where to put the configuration, etc...

aertmann · 2016-11-30T18:14:39Z

@Sebobo: awesome! I'd suggest to start with fixing the policy configuration regression as that's pretty critical.

Regarding thumbnail generation, that belongs in the media package. Configuration for Neos belongs in Neos IMO and the media browser depends on Neos so it's fine using configuration from Neos in it.

Regarding the layouts, that's true however I don't see a way around it. It's just how we re-use it as a standalone module and inside an iframe.

This was a regression neos#159 Related: neos#159

This fixes a regression in neos#159 Related: neos#159

Related: neos#159

Sebobo · 2016-12-11T13:29:19Z

Topic continues in #1305

FEATURE: Node type change

dfeyer added I: WIP P: Neos P: Media Task T: PHP labels Oct 26, 2015

dfeyer self-assigned this Oct 26, 2015

dfeyer force-pushed the task-mediabrowser-module branch from 06adef2 to d46a1d0 Compare November 17, 2015 10:41

dfeyer force-pushed the task-mediabrowser-module branch from d46a1d0 to 3ce7692 Compare November 17, 2015 17:36

neos-project removed the K: Task label Nov 20, 2015

aertmann mentioned this pull request May 20, 2016

FEATURE: Replace existing assets with new version #464

Merged

aertmann mentioned this pull request Jul 22, 2016

TASK: Translate FlashMessages for AssetController in the view #618

Closed

Sebobo self-assigned this Nov 21, 2016

Sebobo force-pushed the task-mediabrowser-module branch from 3ce7692 to 6d8f519 Compare November 23, 2016 15:50

Sebobo removed the I: WIP label Nov 23, 2016

Sebobo unassigned dfeyer and Sebobo Nov 23, 2016

Sebobo force-pushed the task-mediabrowser-module branch 4 times, most recently from c4beffd to e76abe2 Compare November 23, 2016 18:32

johannessteu approved these changes Nov 23, 2016

View reviewed changes

johannessteu changed the title ~~[TASK] Create a new package for the Media Browser~~ TASK: Create a new package for the Media Browser Nov 23, 2016

dfeyer and others added 2 commits November 24, 2016 10:00

[TASK] Create a new package for the Media Browser

22fad0e

This change extract the MediaBrowser from the Neos core package and create an new package name Neos.Media.Browser. Resolves: NEOS-1683

TASK: Rebase media browser on latest master

c807d0d

Sebobo force-pushed the task-mediabrowser-module branch from e76abe2 to c807d0d Compare November 24, 2016 09:07

gerhard-boden suggested changes Nov 24, 2016

View reviewed changes

gerhard-boden self-assigned this Nov 24, 2016

BUGFIX: Do not ouput wrong label when replacing asset

bac9434

gerhard-boden approved these changes Nov 24, 2016

View reviewed changes

Sebobo merged commit 6d08ff9 into neos:master Nov 24, 2016

gerhard-boden removed their assignment Nov 24, 2016

aertmann reviewed Nov 28, 2016

View reviewed changes

Sebobo added a commit to Sebobo/neos-development-collection that referenced this pull request Dec 10, 2016

BUGFIX: Prevent whitespace in breadcrumb

aa899b5

This was a regression neos#159 Related: neos#159

Sebobo added a commit to Sebobo/neos-development-collection that referenced this pull request Dec 10, 2016

BUGFIX: Use new format again for requestPatterns

e27da9d

This fixes a regression in neos#159 Related: neos#159

Sebobo added a commit to Sebobo/neos-development-collection that referenced this pull request Dec 10, 2016

TASK: Adjust media browser package description

1ef78ad

Related: neos#159

Sebobo added a commit to Sebobo/neos-development-collection that referenced this pull request Dec 10, 2016

TASK: Combines both AssetControllers

4301278

Related: neos#159

Sebobo mentioned this pull request Dec 10, 2016

BUGFIX: Fix mediabrowser regressions, improve configuration #1305

Merged

12 tasks

skurfuerst added a commit that referenced this pull request May 3, 2022

Merge pull request #159 from neos/node-type-change

480a4dd

FEATURE: Node type change

TASK: Create a new package for the Media Browser #159

TASK: Create a new package for the Media Browser #159

Conversation

dfeyer commented Oct 26, 2015 • edited by johannessteu Loading

dfeyer commented Oct 26, 2015

dfeyer commented Oct 26, 2015

aertmann commented Oct 27, 2015

dfeyer commented Oct 27, 2015

dfeyer commented Oct 29, 2015

dfeyer commented Nov 17, 2015

kitsunet commented Jan 26, 2016

Sebobo commented Nov 21, 2016

Sebobo commented Nov 23, 2016

johannessteu left a comment

Choose a reason for hiding this comment

dfeyer commented Nov 23, 2016 • edited Loading

Sebobo commented Nov 24, 2016

johannessteu commented Nov 24, 2016

gerhard-boden left a comment

Choose a reason for hiding this comment

Sebobo commented Nov 24, 2016

dfeyer commented Nov 24, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aertmann commented Nov 28, 2016

Sebobo commented Nov 29, 2016

aertmann commented Nov 30, 2016

Sebobo commented Dec 11, 2016

dfeyer commented Oct 26, 2015 •

edited by johannessteu

Loading

dfeyer commented Nov 23, 2016 •

edited

Loading