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

[2.3] Database Media Storage - Design Config Save functions to be Database Media Storage aware #21675

Merged
merged 5 commits into from
May 14, 2019
Merged

Conversation

gwharton
Copy link
Contributor

@gwharton gwharton commented Mar 10, 2019

Description (*)

The functions that process the uploads of the Favicon, Header Image and Transactional Email Logo files do not process the file correctly when in Database Media Storage Mode. This PR fixes that.

Fixed Issues (if relevant)

  1. Database Media Storage - Design Config fails to save transactional email logo correctly #21672: Database Media Storage - Design Config fails to save transactional email logo correctly

Manual testing scenarios (*)

  1. Setup Magento
  2. Change media storage mode to database
  3. Synchronize
  4. Save Settings
  5. Content->Configuration->Edit->Transactional Emails
  6. Upload New Image (but don't save)
  7. Verify temporary email logo exists in pub/media
  8. Verify temporary email logo exists in database
  9. Save Config
  10. Verify email logo exists in pub/media
  11. Verify email logo exists in database

Repeat for favicon and header logo, ensuring that after save, the file exists correctly in the database.

Contribution checklist (*)

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

@magento-engcom-team
Copy link
Contributor

Hi @gwharton. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@gwharton gwharton changed the title [2.3]] Database Media Storage - Design Config Save functions to be Database Media Storage aware [2.3] Database Media Storage - Design Config Save functions to be Database Media Storage aware Mar 10, 2019
@aleron75 aleron75 self-assigned this Mar 11, 2019
@orlangur
Copy link
Contributor

@aleron75 any update on this?

@VasylShvorak
Copy link
Contributor

Hi @gwharton could you resolve merge conflicts?

@gwharton
Copy link
Contributor Author

OK, sometimes github does things that I really don't understand.

So i used the github "Resolve conflict" feature to resolve the conflicts. This appeared to work ok, but if you look at the commit history, the resolve conflicts also merged all changes from 2.3-develop into my PR since the PR was created. This means the commit for this PR is now massive!!!! Arghh.

Will I really be re-committing all of the changes made in the last month in 2.3-develop back in on itself????? I don't know what to do to get out of this.

@aleron75
Copy link
Contributor

If you look at the commit history, the resolve conflicts also merged all changes from 2.3-develop into my PR since the PR was created.

It means that, since you forked the repo, some changes were merged in the main branch and it's normal that your PR now includes those changes because your fork was not up to date anymore.

@gwharton
Copy link
Contributor Author

gwharton commented Apr 24, 2019

Ok. Thats great. For one moment i thought my pr would be "recommitting" all those changes twice. The "files changed" section looks correct.

Copy link
Contributor

@orlangur orlangur left a comment

Choose a reason for hiding this comment

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

Please squash changes into a single commit so that we have perfectly clean history 😉

@ghost ghost assigned orlangur Apr 24, 2019
@gwharton
Copy link
Contributor Author

@orlangur, perhaps I am doing it wrong, I did try and squash them last night into a single commit, but my squashed commit ended up containing all of the changes from the merge of 2.3-develop back into my branch, which I am sure is not what is wanted.

What is the correct way of doing it, when one of the commits, is a merge of 2.3-develop into my branch.

On previous attempts I would have

git checkout branch 
git reset --soft HEAD~XXX
git commit
git push --force origin branch

@orlangur
Copy link
Contributor

@gwharton create a new branch from a fresh 2.3-develop and then git merge --squash from the old branch.

…renamed aswell as the local file in pub/media.
Copy link
Member

@sivaschenko sivaschenko left a comment

Choose a reason for hiding this comment

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

Hi @gwharton , thanks for the pull request, please see my suggestion in the review comment

app/code/Magento/Theme/Model/Design/Backend/File.php Outdated Show resolved Hide resolved
@ghost ghost assigned sivaschenko Apr 27, 2019
@magento-engcom-team
Copy link
Contributor

Hi @sivaschenko, thank you for the review.
ENGCOM-4802 has been created to process this Pull Request

@gwharton
Copy link
Contributor Author

Backport PR at #21676 has been updated inline with this parent PR, and is ready to go once this has been processed.

@gwharton
Copy link
Contributor Author

gwharton commented Apr 29, 2019

Not sure whats going on with the travis build. Not sure its anything to do with me. Looks to be failing attempting to fork a process under php 7.2. I've tried rerunning the travis build twice and it fails with same error. Suspect out of memory during tests.

@sdzhepa
Copy link
Contributor

sdzhepa commented May 7, 2019

✔️ QA Passed

@m2-assistant
Copy link

m2-assistant bot commented May 14, 2019

Hi @gwharton, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

magento-engcom-team pushed a commit to okorshenko/magento2 that referenced this pull request May 14, 2019
@magento-engcom-team magento-engcom-team added this to the Release: 2.3.3 milestone May 14, 2019
@gwharton gwharton deleted the 2.3-develop-dbmediastorage4 branch May 31, 2019 08:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants