@andersonjeccel That’s currently how the scaffold files work. So changing that is a separate change best in another PR.

My suggestion though would be perhaps to “build” the assets/scaffold folder during package release - I am not familiar with that process though and only say this as I found what I thought was some file list builders in the packages folder - so it felt there’s already some automation there that could be expanded.

@driskell The most efficient solution I can think at this moment would be to move the defaults folder into one of the bundles, changing the path from each theme to point to the new location

We probably could move forward without a duplicate instance of it, what do you think?

@andersonjeccel I thought about that but the app folder has a .htaccess to deny all access. So it would, I think, need a brand new package for example mautic/theme-defaults.

https://github.com/mautic/mautic/blob/6.x/app/.htaccess

I think as well this only solves it for the theme defaults - not for any other duplicated files in the scaffold area. So I don't think not doing work here is going to cause more work later. Ideally the solution for @defaults would work for the others too so that's why I thought it better to just keep the status quo and then work on that separately at some point if indeed it becomes an issue. The likelihood of these files changing seems fairly slim though I think much like the other scaffold files. And indeed the current approach has some advantages in that an installation could override and replace these files as they see fit to change the defaults - where bundling them in with core/lib would be a bit harder and require a patch file rather than a simple entry in composer.json (and patch files aren't gonna work well for binary images). So there's likely some larger things to think about

@driskell Got it, makes sense. Let's proceed this way then

Do you have any clue why the PHPUnit test is failing?

@driskell Got it, makes sense. Let's proceed this way then

Do you have any clue why the PHPUnit test is failing?

Sorry I don't - it seems relating to asset download so I thought maybe a randomly failure but unfortunately I don't have access to re-run the tests. But I can try force push a change and see if it works.

@andersonjeccel Yeh it was a random test failure. All pass now!

This doesn't seem right. But I also don't fully understand this Composer magic. Shouldn't this theme be mentioned on the same places as other themes? For example this is where the Skyline theme is mentioned but the @defaults theme is not:

• mautic/composer.json

  Line 82 in 0d57852

  "mautic/theme-skyline": "self.version",

• mautic/app/bundles/CoreBundle/Helper/ThemeHelper.php

  Line 83 in 0d57852

  'skyline',

• mautic/.github/workflows/gitsplit/theme-skyline.json

  Line 4 in 0d57852

  "name": "theme-skyline",

• mautic/.github/workflows/split-monorepo-in-multi-repo.yml

  Line 67 in 0d57852

  - theme-skyline

• https://github.com/search?q=repo%3Amautic%2Fmautic%20skyline&type=code

• mautic/.github/subtree-splitter-config.json

  Line 149 in 0d57852

  "name": "theme-skyline",

It felt like these were not a theme but shared assets so I went the asset right but you could be right it be better to be its own bundle and added to the dependencies of each core theme so updates bring it in. Happy to close in that case but I think it might be beyond my contribution to create that theme as I not familiar with the working of that split stuff and not sure how I would test it. I could try tho as you outlined the files but it would probably be an untested PR 😬

@mollux do you think you could advise here?

I find that it would be better to create @defaults as its own theme repository like the other themes, since the files are located in the themes folder. This approach would help avoid maintaining duplicate files.

@escopecz has already listed the files that need to be updated or created to set up the @defaults theme. It should be named something like theme-defaults, not theme-@defaults, as the repository name can't contain @. There’s no need to test this.

I put manually these files and patched already existing files into Mautic 5.2.1 instance, recreated assets, cleared cache but the issue was not resolved.
@escopecz The @default theme where should be there?

@trianity This @defaults folder from https://github.com/mautic/mautic/tree/5.2.1/themes has to be copied to the <Mautic root>/docroot/themes/ folder.

@trianity This @defaults folder from https://github.com/mautic/mautic/tree/5.2.1/themes has to be copied to the <Mautic root>/docroot/themes/ folder.

Thanks, I can confirm: copying the @defaults folder content to the <Mautic root>/docroot/themes/ folder, the error 500 issue was resolved in the unsubscribe link click.

@trianity This @defaults folder from https://github.com/mautic/mautic/tree/5.2.1/themes has to be copied to the <Mautic root>/docroot/themes/ folder.

also Landing page builder (with blank theme) is working again after copying @defaults to docroot/themes

Or can we move the default theme twig files here? https://github.com/mautic/mautic/tree/6.x/app/bundles/CoreBundle/Resources/views/Theme

Yes, it's possible to move these files to app/bundles/CoreBundle/Resources/views/Theme and then replace with:

-{% extends "@themes/@defaults/html/base.html.twig" %}
+{% extends "@MauticCore/Theme/base.html.twig" %}

I just tested it, and this would be an even better option.

@escopecz @matbcvo I'm wondering if, when the Preference center is enabled in Configuration, if contacts will have permission to access

If you can confirm, this would be nice

@andersonjeccel @escopecz @matbcvo On our platforms we block all app access as nothing needs it. All assets are outside of it in the media folder and I don't recall anything ever needing to access in there. So editors wouldn't see the images if they were referenced from CoreBundle. Granted, templates likely would work. But it's then where to put the images.

In Mautic app is blocked by Apache: https://github.com/mautic/mautic/blob/6.x/app/.htaccess

@driskell I was talking more about the twig files. You are right, the images should be moved to the media directory.

@driskell I was talking more about the twig files. You are right, the images should be moved to the media directory.

Yeh sorry, quick to jump to reply! I will see if I can work on splitting the base templates to CoreBundle then and look at treating these images like the current images are treated.

OK @escopecz @andersonjeccel I've done an initial draft - just gonna convert this to a GitHub Draft though so it's clear as I need to get some testing time which will need to be next week. Let me know if it looks like what you intended though and I'll get the testing done to make sure it all works OK. 👍

Thanks everyone for the feedback so far! In the meantime if anyone reaches here the workaround is to make sure you have @defaults theme in your theme folder if you're running a composer installation. Once this PR is done that won't be needed anymore so just need to remember to remove it but it should be harmless either way.

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 63.45%. Comparing base (3df5f9e) to head (91342dc).
Report is 62 commits behind head on 5.2.

@@            Coverage Diff            @@
##                5.2   #14425   +/-   ##
=========================================
  Coverage     63.44%   63.45%           
- Complexity    34630    34636    +6     
=========================================
  Files          2273     2273           
  Lines        103594   103603    +9     
=========================================
+ Hits          65728    65741   +13     
+ Misses        37866    37862    -4     

I just tested your PR (emails, forms, landing pages, unsubscribe page). No issues found so far, it works as expected. Placeholder images are present and publicly accessible.

@driskell Why getOverridableUrl to retrieve the placeholders?

@driskell Why getOverridableUrl to retrieve the placeholders?

I copied how it was done for the other images in the assets folder - which allow a copy to be made into the media folder to override them. I just went and used the same loading method as the other assets in that folder so I didn’t need to find the right API myself, but I didn’t think too much about it 😬 I can adjust if someone drops a review with what to use instead though. Might be ok though as it still should work and just has added bonus of behaving like the others and allowing overrides

@driskell From what I got, the solution will lead to only one place for managing these placeholders and using this function automatically solves technical things related to security, to get the theme working as expected for end users

Is it right? (I'm really not a PHP dev)

@driskell From what I got, the solution will lead to only one place for managing these placeholders and using this function automatically solves technical things related to security, to get the theme working as expected for end users

Is it right? (I'm really not a PHP dev)

Correct - maintenance of the Mautic product wise - only one location for both the base templates and shared assets. And users can override the templates as per normal I expect and also user can override the assets in their media folders.

The function is used elsewhere so yes I would expect it to be safe and vetted already 😊 If not - one fix will fix it for all usages including existing usages.

@driskell Got it! Thank you for explaining:)

I've just realised the entire assets folder that is now outside of app is also missing from composer installation, which explains some dodgy workarounds on my end. I'll try get some testing done myself on this soon but I will move out of draft since we had one test done already.

I'll then try look separately at the missing assets folder as perhaps that's a scaffold thing - I'll raise an issue for discussion though on best way to resolve as perhaps it needs a new composer package mautic/mautic-core-assets?

Issue on my side, sorry, ignore this

I'm not sure what you meant by assets being outside the app folder. The assets are still located within the app folder and will be included in the Composer-based installation.

I'm not sure what you meant by assets being outside the app folder. The assets are still located within the app folder and will be included in the Composer-based installation.

Sorry. I realise now there is no issue. So I will strike through this. Sorry for the confusion. I'm using Nginx and it looks like we missed allowing access to app/assets - in my head I thought it was in public and meant to be outside app but no it's just an issue on my side!