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

[4.0] Fix media path to banners folder #21189

Merged
merged 8 commits into from
Oct 20, 2019
Merged

Conversation

anuragteapot
Copy link
Contributor

@anuragteapot anuragteapot commented Jul 20, 2018

Issue

Try to create a new banner. And select image.
Error. Server internal error.

Summary of Changes

Fix path in URL.

Testing Instructions

Try to create a new banner. And select image.

Expected result

screenshot from 2018-07-20 08-24-44

Actual result

screenshot from 2018-07-20 08-25-18

Error

Documentation Changes Required

NO

@ghost
Copy link

ghost commented Jul 20, 2018

PR isn't shown at Issue Tracker, so no mark as successfully Test can be done.

@ghost
Copy link

ghost commented Jul 20, 2018

This PR is now shown in Issue Tracker, but as Issue, not as PR.

@dgrammatiko
Copy link
Contributor

@laoneo can you get Kasun to review this. Hardcoding the local adapter doesn't look very promising solution...

@brianteeman
Copy link
Contributor

@laoneo any update on this?

@laoneo
Copy link
Member

laoneo commented Aug 9, 2018

Ups, that slipped through my inbox. With the new media storage API, we get now the question if hardcoded paths should still be supported. Because somebody can disable the local adapter and only work with media data from a cloud provider, this will likely crash again. I'm in favor to not support predefined paths as we store now the last opened folder in the local storage of the user anyway. Opinions?

@kasvith
Copy link
Contributor

kasvith commented Aug 11, 2018

Allon is correct in here, btw as we resolves the URL without adapter prefix even disabling an adapter wont affect articles we use.

About this one need to check whats the url being used to serve the banner, will take a look on pr

@kasvith
Copy link
Contributor

kasvith commented Aug 11, 2018

Ok Provided solution is not acceptable for me, it uses a hardcoded adapter, even though we provide a method to fetch absolute url to a file, we can get the real url without the adapter prefix.

It need to be done with js like @dgramatikko did with articles

@brianteeman
Copy link
Contributor

, we can get the real url without the adapter prefix.

As long as it is not an absolute URL ;)

@anuragteapot
Copy link
Contributor Author

@kasvith Thanks for your review :)

@dgrammatiko
Copy link
Contributor

dgrammatiko commented Aug 11, 2018

FWIW I didn't do any magic tricks on the other fields, the reality is that the other fields don't use the folder option in the XML. Quite honestly I don't see how some client-side code can help up pick the right adapter. I think there must be a trait with some logic that handles that (some basic checks I guess), or just add one more option to the XML: adapter...

@ghost ghost added the J4 Issue label Apr 5, 2019
@ghost ghost removed the J4 Issue label Apr 13, 2019
@Quy Quy added the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Jul 4, 2019
@roland-d
Copy link
Contributor

@Anu1601CS Is this ready to be tested?

@ghost ghost changed the title [4.0] Fix media path to banners folder. [4.0] Fix media path to banners folder Sep 1, 2019
@anuragteapot
Copy link
Contributor Author

@Anu1601CS Is this ready to be tested?

This is not perfect fix for this. But, it works for now.

We can open a new issue regarding not to hard code local adapter.

@roland-d
Copy link
Contributor

roland-d commented Sep 2, 2019

@wilsonge Are you OK with fixing the hard coded local adapter in a separate PR or should it be done in this PR?

@Schmidie64
Copy link
Contributor

Schmidie64 commented Sep 2, 2019

I have tested this item ✅ successfully on 571a2e0

For me the patch works fine, without any obvious problems.
System:
PHP Build On: Windows NT 10.0 build 18362 (Windows 10) i586
10.1.37-MariaDB
PHP-Version: 7.3.0
Webserver: Apache/2.4.37 (Win32) OpenSSL/1.1.1a PHP/7.3.0
Joomla! Version: Joomla! 4.0.0-alpha12-dev Development [ Amani ] 19-August-2019 13:40 GMT
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.132 Safari/537.36


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21189.

@Lyro274
Copy link

Lyro274 commented Sep 2, 2019

I have tested this item ✅ successfully on 571a2e0

I followed the instructions provided by @Anu1601CS. Everything worked fine.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21189.

@ghost
Copy link

ghost commented Sep 2, 2019

Status "Ready To Commit".

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 2, 2019
@Quy Quy removed the Updates Requested Indicates that this pull request needs an update from the author and should not be tested. label Sep 2, 2019
@wilsonge
Copy link
Contributor

wilsonge commented Sep 2, 2019

@roland-d i'd rather fix it if it's not some insane amount of work

@roland-d
Copy link
Contributor

roland-d commented Sep 2, 2019

@Anu1601CS Can you please fix the issue in this PR?

@anuragteapot
Copy link
Contributor Author

@Anu1601CS Can you please fix the issue in this PR?

Ok i will try.

@Quy Quy added Updates Requested Indicates that this pull request needs an update from the author and should not be tested. and removed RTC This Pull Request is Ready To Commit labels Sep 20, 2019
@KishoriBKarale
Copy link

I have tested this item ✅ successfully on 571a2e0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21189.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Oct 19, 2019
@shraddhaSankpal27
Copy link

I have tested this item ✅ successfully on 571a2e0


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/21189.

@Quy Quy added Updates Requested Indicates that this pull request needs an update from the author and should not be tested. and removed RTC This Pull Request is Ready To Commit Updates Requested Indicates that this pull request needs an update from the author and should not be tested. labels Oct 19, 2019
@laoneo laoneo merged commit 1137b0f into joomla:4.0-dev Oct 20, 2019
@laoneo
Copy link
Member

laoneo commented Oct 20, 2019

Thanks!

@wilsonge
Copy link
Contributor

Captured doing a proper fix for the issue #26750 which doesn't block any potential cloud providers here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Updates Requested Indicates that this pull request needs an update from the author and should not be tested.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet