Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allowed file extensions configuration for installed theme #6436

Merged
merged 6 commits into from Nov 10, 2018

Conversation

Projects
4 participants
@kuzmany
Copy link
Contributor

kuzmany commented Aug 8, 2018

Please be sure you are submitting this against the staging branch.

Q A
Bug fix?
New feature?
Automated tests included?
Related user documentation PR URL
Related developer documentation PR URL
Issues addressed (#s or URLs) #6348
BC breaks?
Deprecations?

Description:

This update bring allowed extension for theme installation.
Also bring new theme settings pane to configurations

image

Steps to reproduce bug:

  1. Try upload theme test theme https://1drv.ms/u/s!AvXHYm0qUE5it5xO55-1UfRosDeNrw . This test theme has test.php file in HTML directory
  2. Go to themes/test/html/ - see no php file was imported (security reason) - #6348 (comment)

Steps to test this PR:

  1. Apply PR, clear cache and go to configurations > see new Theme Settings pane
  2. Allow php extensions
  3. Try upload again theme test theme https://1drv.ms/u/s!AvXHYm0qUE5it5xO55-1UfRosDeNrw . This test theme has test.php file in HTML directory
  4. Go to themes/test/html/ - see php file was imported

@kuzmany kuzmany added this to the 2.15.0 milestone Aug 8, 2018

@kuzmany kuzmany self-assigned this Aug 8, 2018

@npracht
Copy link
Member

npracht left a comment

Just tested, it works properly

Was ok for using the feature, didn't notice the issue on config (see last comment). I'll retest after fixing what has been pointed out.

@Enc3phale
Copy link
Contributor

Enc3phale left a comment

It's seems there is a bug.
Theme Settings is in conflict with System Settings.
System Settings is never saved because Theme Settings erase previous data.

'bundle' => 'CoreBundle',
'formAlias' => 'themeconfig',
'formTheme' => 'MauticCoreBundle:FormTheme\Config',
'parameters' => $event->getParametersFromConfig('MauticCoreBundle'),

This comment has been minimized.

@Enc3phale

Enc3phale Oct 16, 2018

Contributor

I think bug is here, parameters is reloaded on theme tab.
Maybe theme tabs must be merged with system tab?

This comment has been minimized.

@kuzmany

kuzmany Oct 16, 2018

Author Contributor

I'll fix it.

This comment has been minimized.

@kuzmany

kuzmany Oct 16, 2018

Author Contributor

More info about bug: #5577
Already fixed. Thanks 👍

@npracht npracht added this to Pending feedback in Testing 2.15.0 Oct 16, 2018

@Enc3phale
Copy link
Contributor

Enc3phale left a comment

Tested and it's good! Thanks!

@npracht
Copy link
Member

npracht left a comment

Feature is still working :)

@kuzmany

This comment has been minimized.

Copy link
Contributor Author

kuzmany commented Oct 17, 2018

@escopecz Just confirm, If my solution works. It's little different way like you introduced here #5577 But my test works.

I am talking about https://github.com/mautic/mautic/pull/6436/files#diff-c19cce586056ec354547b9a4c3e7f127R60
https://github.com/mautic/mautic/pull/6436/files#diff-1ef3c88d8b8cf749626c46ff9703b103R38

@kuzmany kuzmany moved this from Pending feedback to Tested once in Testing 2.15.0 Oct 17, 2018

@npracht npracht moved this from Tested once to Ready to commit in Testing 2.15.0 Oct 23, 2018

@Woeler Woeler merged commit 1e34435 into mautic:staging Nov 10, 2018

2 checks passed

Scrutinizer Analysis: 7 updated code elements – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Testing 2.15.0 automation moved this from Ready to commit to Merged Nov 10, 2018

@Woeler Woeler removed the Ready To Commit label Nov 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.