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

Attempt to fix issue #3059 #3060

Merged
merged 7 commits into from
May 30, 2018
Merged

Attempt to fix issue #3059 #3060

merged 7 commits into from
May 30, 2018

Conversation

jannejava
Copy link
Contributor

If no image is uploaded the setting’s value become null and overwrites the previous value.

If no image is uploaded the setting’s value become null and overwrites the previous value.
$content = $this->getContentBasedOnType($request, 'settings', (object) [
'type' => $setting->type,
'field' => str_replace('.', '_', $setting->key),
'field' => $field,
'details' => $setting->details,
'group' => $setting->group,
]);

Copy link
Collaborator

@emptynick emptynick May 2, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move your condition from line 91 to line 101 and use
if ($setting->type == 'image' && $content == null)
This makes everything else unnecessary.

Copy link
Contributor Author

@jannejava jannejava left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, I see what you mean. Should I do another pull request?

@emptynick
Copy link
Collaborator

You just can push to your branch, it will update here automatically.

@jannejava
Copy link
Contributor Author

Did the update of the pull request work?

@fletch3555
Copy link
Collaborator

Yes it did.

Also, please try to avoid committing whitespace changes (new lines, indentation/spacing changes, etc). All they do is make the diff harder to read, and make StyleCI revert them after this gets merged. I understand some IDEs do this automatically, but you can just be more selective in what you commit (e.g. you can select specific lines instead of entire files).

@jannejava
Copy link
Contributor Author

Sorry for that! It must be php-cs-fixer, I have it set on PSR-2 but you are maybe using Symfony style? Anyway, I will try to disable it for this project.

Do you have any idea if this would fix the disappearing images on the settings page? (it’s a little annoying, when the images disappear ;-))

@emptynick
Copy link
Collaborator

Yep thats working

Copy link
Contributor

@cyrildewit cyrildewit left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants