The attached patches are from a coder review. There are still some issues that coder is reporting- I chose to ignore a few that seemed low priority for the moment. I broke this patch up into individual patches for the sheer size of it. It is probably the case that some of these will need to be broken off into their own issues- I'm not sure what the usual protocol is around handling this number of changes.

CommentFileSizeAuthor
#3 streamwrapper.diff16.33 KBarthurf
#3 media.browser.inc_.diff6.42 KBarthurf
#3 media.filter.inc_.diff7.77 KBarthurf
#3 media.xml_.inc_.diff2.15 KBarthurf
#3 monolithic.diff80.44 KBarthurf
#1 coder_monlithic.diff80.25 KBarthurf
MediaReadOnlyStreamWrapper.inc_.diff16.14 KBarthurf
mediafield.install.diff613 bytesarthurf
MediaEntityTranslationHandler.inc_.diff669 bytesarthurf
MediaBrowserPluginInterface.inc_.diff1.24 KBarthurf
MediaBrowserPlugin.inc_.diff1.32 KBarthurf
media.xml_.inc_.diff2.15 KBarthurf
media.views_.inc_.diff2.6 KBarthurf
media.variables.inc_.diff6.66 KBarthurf
media.theme_.inc_.diff2.69 KBarthurf
media.test.diff425 bytesarthurf
media.pages_.inc_.diff917 bytesarthurf
media.module.diff17.15 KBarthurf
media.media_.inc_.diff1007 bytesarthurf
media.install.diff3.23 KBarthurf
media.inc_.diff1.19 KBarthurf
media.filter.inc_.diff7.77 KBarthurf
media.fields.inc_.diff1.01 KBarthurf
media.entity.test.diff1.01 KBarthurf
media.browser.inc_.diff6.42 KBarthurf
media.admin_.inc_.diff3.38 KBarthurf
media_views_plugin_style_media_browser.inc_.diff1.41 KBarthurf
media_internet.media_.inc_.diff538 bytesarthurf
media_internet.api_.php_.diff834 bytesarthurf

Comments

arthurf’s picture

arthurf commented
StatusFileSize
new coder_monlithic.diff80.25 KB

Here's the monolithic version of all the above patches

ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Needs work

Awesome! thanks a lot:)
Just a few issues i found

+++ b/includes/MediaReadOnlyStreamWrapper.incundefined
@@ -470,23 +429,46 @@ abstract class MediaReadOnlyStreamWrapper implements DrupalStreamWrapperInterfac
+   * Implements unlink.

Should be
Implements DrupalStreamWrapperInterface::unlink()

+++ b/includes/MediaReadOnlyStreamWrapper.incundefined
@@ -470,23 +429,46 @@ abstract class MediaReadOnlyStreamWrapper implements DrupalStreamWrapperInterfac
+   * Implements rename.

Same for the rest

+++ b/includes/media.browser.incundefined
@@ -214,10 +234,14 @@ function media_attach_browser_js(&$element) {
+    'browserUrl' => url('media/browser', array(
+      'query' => array('render' => 'media-popup'),
+      )
+    ),
+    'styleSelectorUrl' => url('media/-media_id-/format-form', array(
+      'query' => array('render' => 'media-popup'),
+      )
+    ),

Seems you missed that:) indentation is a bit off

+++ b/includes/media.filter.incundefined
@@ -499,13 +506,13 @@ function media_filter_invalidate_caches($fid = FALSE) {
+ * @param ing $fid

should be int:)

+++ b/includes/media.xml.incundefined
@@ -1,23 +1,24 @@
+ * @return objectl

i guess 1 is typo?

arthurf’s picture

arthurf commented
StatusFileSize
new monolithic.diff80.44 KB
new media.xml_.inc_.diff2.15 KB
new media.filter.inc_.diff7.77 KB
new media.browser.inc_.diff6.42 KB
new streamwrapper.diff16.33 KB

Here are the modified individual patches and the monolithic. I'm not sure what coder wants for that array structure in media.browser.inc- it wasn't flagging that before, but I indented that further and it doesn't complain.

arthurf’s picture

arthurf commented
Status: Needs work » Needs review
ParisLiakos’s picture

ParisLiakos commented
Status: Needs review » Fixed

aw yay, that was awesome, thanks a lot..committed with just a couple corrections!
http://drupalcode.org/project/media.git/commit/c7ef484

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.