It would be great if you could take another look at this as well @dgrammatiko

I have tested this item ✅ successfully on 732952d

I've successfully verified that the media/plg_editors_codemirror/LICENSE file is present after a build with this PR, which is not the case without this PR, and that there are no unexpected other differences compared to a build with the current 6.2-dev branch.

I have tested this item ✅ successfully on 732952d

I've successfully verified that the media/plg_editors_codemirror/LICENSE file is present after a build with this PR, which is not the case without this PR, and that there are no unexpected other differences compared to a build with the current 6.2-dev branch.

I have tested this item ✅ successfully on 732952d

I've successfully verified that the media/plg_editors_codemirror/LICENSE file is present after a build with this PR, which is not the case without this PR, and that there are no unexpected other differences compared to a build with the current 6.2-dev branch. I've tested on Linux.

The failing system tests are not related to this PR.

why do we need a specific method for inclufing the licence for codemirror but not for all other packages? Surely this is a bandage and not a fix to why the code that works for all other packages doesnt work for codemirror. Or is there something specific about codemirror causing this break? Its important to understand the reason so that when we introduce a new package from a new vendor in the future we know if we need to create a specific code for the licence or not

why do we need a specific method for inclufing the licence for codemirror but not for all other packages? Surely this is a bandage and not a fix to why the code that works for all other packages doesnt work for codemirror. Or is there something specific about codemirror causing this break? Its important to understand the reason so that when we introduce a new package from a new vendor in the future we know if we need to create a specific code for the licence or not

@brianteeman The reason why codemirror is special here is because in our composer.json we do not have one dependency for codemirror but several, and there is no master dependency above it, so we have several source folders in node_modules but onmly one target folder in media/vendor. That's why it needs that special treatment.

why do we need a specific method for inclufing the licence for codemirror but not for all other packages? Surely this is a bandage and not a fix to why the code that works for all other packages doesnt work for codemirror. Or is there something specific about codemirror causing this break? Its important to understand the reason so that when we introduce a new package from a new vendor in the future we know if we need to create a specific code for the licence or not

@brianteeman The reason why codemirror is special here is because in our package.json we do not have one dependency for codemirror but several, and there is no master dependency above it, so we have several source folders in node_modules but onmly one target folder in media/vendor. That's why it needs that special treatment.

why do we need a specific method for inclufing the licence for codemirror but not for all other packages? Surely this is a bandage and not a fix to why the code that works for all other packages doesnt work for codemirror. Or is there something specific about codemirror causing this break? Its important to understand the reason so that when we introduce a new package from a new vendor in the future we know if we need to create a specific code for the licence or not

@brianteeman The reason why codemirror is special here is because in our package.json we do not have one dependency for codemirror but several, and there is no master dependency above it, so we have several source folders in node_modules but onmly one target folder in media/vendor. That's why it needs that special treatment.

See https://github.com/joomla/joomla-cms/blob/6.2-dev/package.json#L37-L51 .

why do we need a specific method for inclufing the licence for codemirror but not for all other packages? Surely this is a bandage and not a fix to why the code that works for all other packages doesnt work for codemirror. Or is there something specific about codemirror causing this break? Its important to understand the reason so that when we introduce a new package from a new vendor in the future we know if we need to create a specific code for the licence or not

@brianteeman The reason why codemirror is special here is because in our package.json we do not have one dependency for codemirror but several, and there is no master dependency above it, so we have several source folders in node_modules but onmly one target folder in media/vendor. That's why it needs that special treatment.

See https://github.com/joomla/joomla-cms/blob/6.2-dev/package.json#L37-L51 .

P.S.: The LICENSE file is the same for all of these.

Thank you for the explanation

LGTM

Wouldn't it be sufficient to align these two?

is this code redundant now?

I've experimented a bit with your idea. It's possible, but what benefit do we gain from it? @heelc29

Wouldn't it be sufficient to align these two?
If I remove /js from that line, I have to manually re-add it to every call in the compileCodemirror function.

joomla-cms/media_source/plg_editors_codemirror/builder.mjs

Line 66 in 2d1c08e

const destBasePath = 'media/vendor/codemirror/js';

If I add /jsadditionally here, I never get a clean starting point again, and the media/vendor/codemirror folder always remains present after the first build.

joomla-cms/media_source/plg_editors_codemirror/builder.mjs

Line 105 in 2d1c08e

const vendorPath = 'media/vendor/codemirror';

In this specific builder we are handling two different resources: plg_edtior_codemirror and vendor\codemirror. The first folder is cleanly removed through the parent call await super.clear();, and then we remove the vendor\codemirror additionally.

I've experimented a bit with your idea. It's possible, but what benefit do we gain from it? @heelc29

Wouldn't it be sufficient to align these two?
If I remove /js from that line, I have to manually re-add it to every call in the compileCodemirror function.

joomla-cms/media_source/plg_editors_codemirror/builder.mjs

Line 66 in 2d1c08e

const destBasePath = 'media/vendor/codemirror/js';

If I add /jsadditionally here, I never get a clean starting point again, and the media/vendor/codemirror folder always remains present after the first build.

joomla-cms/media_source/plg_editors_codemirror/builder.mjs

Line 105 in 2d1c08e

const vendorPath = 'media/vendor/codemirror';

In this specific builder we are handling two different resources: plg_edtior_codemirror and vendor/codemirror. The first folder is cleanly removed through the parent call await super.clear();, and then we remove the vendor/codemirror additionally.

I've experimented a bit with your idea. It's possible, but what benefit do we gain from it? @heelc29
If I remove /js from that line, I have to manually re-add it to every call in the compileCodemirror function.

Wouldn't it be sufficient to align these two?

joomla-cms/media_source/plg_editors_codemirror/builder.mjs

Line 66 in 2d1c08e

const destBasePath = 'media/vendor/codemirror/js';

If I add /jsadditionally here, I never get a clean starting point again, and the media/vendor/codemirror folder always remains present after the first build.

joomla-cms/media_source/plg_editors_codemirror/builder.mjs

Line 105 in 2d1c08e

const vendorPath = 'media/vendor/codemirror';

In this specific builder we are handling two different resources: plg_edtior_codemirror and vendor/codemirror. The first folder is cleanly removed through the parent call await super.clear();, and then we remove the vendor/codemirror additionally.

I've experimented a bit with your idea. It's possible, but what benefit do we gain from it? @heelc29
If I remove /js from that line, I have to manually re-add it to every call in the compileCodemirror function.

Wouldn't it be sufficient to align these two?

joomla-cms/media_source/plg_editors_codemirror/builder.mjs

Line 66 in 2d1c08e

const destBasePath = 'media/vendor/codemirror/js';

If I add /jsadditionally here, I never get a clean starting point again, and the media/vendor/codemirror folder always remains present after the first build.

joomla-cms/media_source/plg_editors_codemirror/builder.mjs

Line 105 in 2d1c08e

const vendorPath = 'media/vendor/codemirror';

In this specific builder we are handling two different resources: plg_edtitors_codemirror and vendor/codemirror. The first folder is cleanly removed through the parent call await super.clear();, and then we remove the vendor/codemirror additionally.

is this code redundant now?

joomla-cms/build/build-modules-js/settings.json

Lines 641 to 644 in 2d1c08e

	"@codemirror/view": {
	"name": "codemirror",
	"licenseFilename": "LICENSE"
	},

@brianteeman No, see my description of the PR

Added a copy method to CodemirrorModuleBuilder that copies the CodeMirror license file to the target location if specified in the build settings (settings.json).

is this code redundant now?

joomla-cms/build/build-modules-js/settings.json

Lines 641 to 644 in 2d1c08e

	"@codemirror/view": {
	"name": "codemirror",
	"licenseFilename": "LICENSE"
	},

@brianteeman No, see my description of the PR

Added a copy method to CodemirrorModuleBuilder that copies the CodeMirror license file to the target location if specified in the build settings (settings.json).

We could specify the path here, but since this would be the only instance in the entire build process not to define the license file in the global settings, it would likely create maintenance friction in the long run.

is this code redundant now?

joomla-cms/build/build-modules-js/settings.json

Lines 641 to 644 in 2d1c08e

	"@codemirror/view": {
	"name": "codemirror",
	"licenseFilename": "LICENSE"
	},

@brianteeman No, see my description of the PR

Added a copy method to CodemirrorModuleBuilder that copies the CodeMirror license file to the target location if specified in the build settings (settings.json).

We could specify the path directly in the method, but since this would be the only instance in the entire build process not to define the license file in the global settings, it would likely create maintenance friction in the long run.

we could say this is the only licence that needs special handling outside of the global settings but I get your point ;)

I've experimented a bit with your idea. It's possible, but what benefit do we gain from it? @heelc29 If I remove /js from that line, I have to manually re-add it to every call in the compileCodemirror function.

If I add /jsadditionally here, I never get a clean starting point again, and the media/vendor/codemirror folder always remains present after the first build.

joomla-cms/media_source/plg_editors_codemirror/builder.mjs

Line 105 in 2d1c08e

const vendorPath = 'media/vendor/codemirror';

In this specific builder we are handling two different resources: plg_edtitors_codemirror and vendor/codemirror. The first folder is cleanly removed through the parent call await super.clear();, and then we remove the vendor/codemirror additionally.

@LadySolveig The advantage would be that you would only need to adjust this line, rather than having to extend the copy method.

The CodemirrorModuleBuilder writes only in the js subfolder of media/vendor/codemirror so it should be enough to clear this folder.

The VendorModuleBuilder already ensures a clean starting point for media/vendor/*.

@heelc29 I see, you mean that since the entire vendor folder is deleted anyway...IF I run a complete new build with all. However, it will only be cleared up completely then when you run npm run -- --all. Not when you run the individual builder with npm run -- -n plg_editors_codemirror. Or am I missing something?

@heelc29 I see, you mean that since the entire vendor folder is deleted anyway...IF I run a complete new build with all. However, it will only be cleared up completely then when you run npm run -- --all. Not when you run the individual builder with npm run -- -n plg_editors_codemirror. And also only copied with build all. Or am I missing something?

@heelc29 I see, you mean that since the entire vendor folder is deleted anyway...IF I run a complete new build with all. However, it will only be cleared up completely then when you run npm run -- --all. Not when you run the individual builder with npm run -- -n plg_editors_codemirror. And also only copied with build all. Or am I missing something?

To be honest, I'm quite pragmatic about this. We can also align the paths like you suggested if we're willing to accept the little inconsistency in that specific case.

The more I think about it, the more I find your solution to be really clean. If other files need to be added in the future, we'll need to touch the builder anyway. Your suggestion of aligning the paths only removes what the specific builder creates, sounds straight forward.

The more I think about it, the more I find your solution to be really clean. If other files need to be added in the future, we'll need to touch the builder anyway. Your suggestion of aligning the paths only removes what the specific builder creates, sounds straight forward.

Thank you very much!

Weird ... I had successfully tested this PR, and after that were no changes except of clean branch updates, but now after it has been merged into 6.2-dev it does not work anymore for me on a current 6.2-dev branch which includes this PR on the same environment (Linux) as used for my successful test. The media/vendor/codemirror/LICENSE file does not exist after composer installand npm ci. Same for packages built with build/build.php, the file is not in the package.

Can it be that the Windows fix from the other PR has changed something here? From code reading I would not expect that, but I don't have any other idea now.

Weird ... I had successfully tested this PR, and after that were no changes except of clean branch updates, but now after it has been merged into 6.2-dev it does not work anymore for me on a current 6.2-dev branch which includes this PR on the same environment (Linux) as used for my successful test. The media/vendor/codemirror/LICENSE file does not exist after composer installand npm ci. Same for packages built with build/build-php, the file is not in the package.

Weird ... I had successfully tested this PR, and after that were no changes except of clean branch updates, but now after it has been merged into 6.2-dev it does not work anymore for me on a current 6.2-dev branch which includes this PR on the same environment (Linux) as used for my successful test. The media/vendor/codemirror/LICENSE file does not exist after composer installand npm ci. Same for packages built with build/build-php, the file is not in the package.

Can it be that the Windows fix from the other PR has changed something here? From code reading I would not expect that, but I don't have any other idea now.

same problem here after a cvlean checkout followed by composer install npm install and i am on windows

I have just tested it again on windows and LICENSE is placed into media/plg_editors_codemirror...

Ah .. I see .. it's copied to a different place, not the media/vendor/codemirror like it was before but the media/plg_editors_codemirror. Of course that's fine and a better place than before, I think. So the old place is not a false alarm when I run my deleted files check.

Ah .. I see .. it's copied to a different place, not the media/vendor/codemirror like it was before but the media/plg_editors_codemirror. So the old place is not a false alarm when I run my deleted files check.

Hmm, but e.g. for TinyMCE, the file license.md is still placed in the media/vendor/tinymce folder and not in the media/plg_editors_tinymce/ folder.

But for Codemirror it is now media/plg_editors_codemirror/ and not media/vendor/codemirror.

That seems inconsistent to me.

yes I see that - its definitely in the wrong place. the licence must be in the vendor/codemirror folder

@brianteeman Could you test PR #47823 ? For me it fixes the issue witht eh LICENSE file without causing any other differences.