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.1] Media web service implementation #34314

Closed
wants to merge 34 commits into from
Closed

[4.1] Media web service implementation #34314

wants to merge 34 commits into from

Conversation

pjdevries
Copy link
Contributor

@pjdevries pjdevries commented May 31, 2021

Joomla-Mono-Vertical-logo-light-background-en.png.base64.txt
Vertical-logo-light-background-en.png.base64.txt

Pull Request for Issue #34224 .

Summary of Changes

My take on a media web service implementation. It relies on com_media's ApiModel to provide the logic. Two service models, MediumModel and MediaModel, provide wrappers around ApiModel to provide an interface as expected by the standard web service base classes.

The service recognizes the following endpoints:

GET {{base_path}}/api/index.php/v1/media
GET {{base_path}}/api/index.php/v1/media/{path}
POST {{base_path}}/api/index.php/v1/media
PATCH {{base_path}}/api/index.php/v1/media/{path}
DELETE {{base_path}}/api/index.php/v1/media/{path}

{path} parameters in the above endpoints represent paths of existing files or folders, relative to the media base folder (images by default). They are used in single item requests instead of the usual numeric item id, since media items don't have id's.

The web service mostly recognizes the same json request parameters as the com_media ApiModel. These are:

path: A file or folder path wherever appropriate. For instance, in case of the list GET request it points to the base folder, from which to retrieve the list of files and/or folders. Or the destination of a file or folder for a POST or PATCH request.

url: Instructs com_media to include a separate url attribute in the resulting file objects. Has no effect on folder objects.

temp: Instructs com_media to include a separate tempUrl attribute in the resulting file objects. I have no idea what the added value for this parameter is. Has no effect on folder objects.

content: Media content in base64 format for POST or PATCH requests.

In places where path is used, it can be prefixed with adapter: to access files managed using that specific adapter. For eaxample: local-images:/banners/osmbanner1.png, dpdropbox-Dropbox:/fo/bar/baz.jpg, etc. If no adapter is specified, local-images is assumed.

These are issues I struggled with during the implementation:

  • The web service API heavily relies on the notion that all data comes from a database.
  • Media files have no database entries and thus no record id.
  • LocalAdapter::createFolder() ignores the result of File::create(), so the sweb service has no clue if creating a new file or folder succeeded.
  • LocalAdapter sanitizes file names, so creating files or folders with questionable names (according to some) should not be a problem. However, the input PATH filter rejects questionable, but otherwise perfectly valid filenames (see [4.0] Why does the path filter not allow spaces in filenames? #34279).
  • LocalAdapter file/folder manipulation methods do not behave very consistently:
    • createFile() and createFolder() return a file or folder basename.
    • move() returns a path (directory + file-/foldername) with leading slash.
    • updateFile(), moveFile() and moveFolder() return nothing.
  • Exceptions thrown by underlying code, com_media's ApiModel in this case, are not handled properly by the default web service logic and result in 500 errors. I encountered similar behaviour before with other web services.

Testing Instructions

Discover and enable the media web service plugin.

Obtain an active Joomla Api Token.

Run the following cURL commands, replacing the following and other, similar place markers:

  • [your J4 website] with the name of your website
  • [your user token] with the above mentioned Joomla Api Token.
  • [your path] with the path of a file or folder to request or manipulate, relative to the base media folder (i.e. images).

Care must be taken that path names comply with the overeager input PATH filter.

The two attached base64 encoded files can be used to test POST and PATCH requests.

GET list
curl --location --request GET 'https://[your J4 website]/api/index.php/v1/media' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]'
GET item
curl --location --request GET 'https://[your J4 website]/api/index.php/v1/media/banners/banner.jpg' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]'
POST new item
curl --location --request POST 'https://[your J4 website]/api/index.php/v1/media' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]' \
--data-raw '{
    "path": "joomla_logo/Vertical-logo-light-background-en.png",
    "content": "[your base64 content]"
}'
PATCH update existing item
curl --location --request PATCH 'https://[your J4 website]/api/index.php/v1/media/**[your file or folder]**' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]' \
--data-raw '{
    "path": "[your new path]",
    "content": "[your new base64 content]"
}'
DELETE remove existing item
curl --location --request DELETE 'https://[your J4 website]/api/index.php/v1/media/[your file or folder]' \
--header 'Content-Type: application/json' \
--header 'X-Joomla-Token: [your user token]'

Documentation Changes Required

There is no documentation yet.

brianteeman and others added 12 commits May 30, 2021 15:45
* [4.0] Changing title to tooltip fpr template preview
* [4.0] mod_popular with disabled hits

When hits are disabled for articles it makes no sense to display the Most Read Articles module in the site or admin as the contents will never be updated.

This PR changes the output of the module so that instead of a list of non-updating articles a message is displayed.
* Use MVCFactory to create models

* CS
@alikon
Copy link
Contributor

alikon commented Jun 1, 2021

maybe i'm doing something wrong

image

@pjdevries
Copy link
Contributor Author

@alikon The error message is probably misleading. My guess is, it's caused by an invalid path. That should be a complete destination path, relative to the media base folder (images), including filename with extension.

@alikon
Copy link
Contributor

alikon commented Jun 1, 2021

ouch i've omitted the filename from the body path....i'll retry....

@alikon
Copy link
Contributor

alikon commented Jun 1, 2021

I have tested this item ✅ successfully on 8466992


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

@alikon alikon requested a review from wilsonge June 1, 2021 19:23
@pjdevries
Copy link
Contributor Author

@alikon and/or @wilsonge I discovered a small issue with Media Manager's path settings in combination with a default FileSystem - Local configuration (see #34358). This also reflects on this media web service implementation. I have a fix, but what to do: first merge this PR and then add a separate PR for the fix or add the fix to this PR?

@alikon
Copy link
Contributor

alikon commented Jun 2, 2021

as long as this pr it'only tested by me ... than better you'll complete in this pr

@pjdevries
Copy link
Contributor Author

as long as this pr it'only tested by me ... than better you'll complete in this pr

Consider it done.

@pjdevries
Copy link
Contributor Author

As a result from the discussions in #34364, I believe some adjustments are necessary. The current implementation does not take multiple adapters into account. I'll ponder a bit on a possible solution and then get back.

@alikon
Copy link
Contributor

alikon commented Jun 3, 2021

sure ...
cannot help too much i'm a complete com_media dumb 😨

@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.1 milestone Sep 27, 2021
@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Sep 27, 2021
@bembelimen bembelimen added Language Change This is for Translators PR-4.1-dev Updates Requested Indicates that this pull request needs an update from the author and should not be tested. Webservices labels Sep 27, 2021
@bembelimen bembelimen added this to the Joomla 4.1 milestone Sep 27, 2021
@bembelimen bembelimen removed RTC This Pull Request is Ready To Commit Language Change This is for Translators Updates Requested Indicates that this pull request needs an update from the author and should not be tested. PR-4.1-dev Webservices labels Sep 27, 2021
@joomla-cms-bot joomla-cms-bot removed this from the Joomla 4.1 milestone Sep 27, 2021
@bembelimen
Copy link
Contributor

Update labels 2


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

@bembelimen bembelimen added this to the Joomla 4.1 milestone Sep 27, 2021
@bembelimen bembelimen added Language Change This is for Translators PR-4.1-dev Updates Requested Indicates that this pull request needs an update from the author and should not be tested. Webservices labels Sep 27, 2021
@alikon
Copy link
Contributor

alikon commented Sep 27, 2021

@pjdevries can you please check pjdevries/joomla-cms#2

@pjdevries
Copy link
Contributor Author

I'm very sorry, but like I mentioned before, I'm able to spend any time on this at the moment.

@bembelimen
Copy link
Contributor

As @pjdevries has no time to finish it, would you @alikon take over and create a new PR with this code + your fixes to Joomla! 4.1-dev branch?
Then we can close this one and finish it in the new one, as I would really like to have this in 4.1.

@laoneo
Copy link
Member

laoneo commented Sep 29, 2021

I can also take it over, as I can spend some time on it in the next week.

@pjdevries
Copy link
Contributor Author

Thanx @laoneo. I would appreciate that.

@alikon
Copy link
Contributor

alikon commented Sep 29, 2021

thanks @laoneo please go ahead, i'll help with test

@laoneo laoneo mentioned this pull request Oct 8, 2021
@laoneo
Copy link
Member

laoneo commented Oct 8, 2021

Created a new pr #35788 which fixed most of the issue from @PhilETaylor. System tests are not yet finished and it also needs some more stabilization. I'm on holiday next week, so will not do much more in that time.

@alikon
Copy link
Contributor

alikon commented Oct 8, 2021

closing in favor of #35788

@alikon alikon closed this Oct 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Language Change This is for Translators Updates Requested Indicates that this pull request needs an update from the author and should not be tested. Webservices
Projects
None yet
Development

Successfully merging this pull request may close these issues.