The module, after some minor configuration from the user, will take video information from YouTube's latest API and update or create an entity. The user choose which entity type the information will update or create. The user also chooses which fields will be updated with what information from YouTube. It also implements hook_cron to run on its own.

My project page can be found here: https://www.drupal.org/sandbox/shaunfrisbee/2575089

The git clone command would be:

git clone --branch 7.x-1.x http://git.drupal.org/sandbox/shaunfrisbee/2575089.git youtube_import

This is my first project that I am actually submitting to the Drupal world so I have no references. Any comments, suggestions, or advice is welcome.

Comments

shaunfrisbee created an issue. See original summary.

PA robot’s picture

PA robot commented
Status: Needs review » Needs work

There are some errors reported by automated review tools, did you already check them? See http://pareview.sh/pareview/httpgitdrupalorgsandboxshaunfrisbee2575089git

We are currently quite busy with all the project applications and we prefer projects with a review bonus. Please help reviewing and put yourself on the high priority list, then we will take a look at your project right away :-)

Also, you should get your friends, colleagues or other community members involved to review this application. Let them go through the review checklist and post a comment that sets this issue to "needs work" (they found some problems with the project) or "reviewed & tested by the community" (they found no major flaws).

I'm a robot and this is an automated message from Project Applications Scraper.

iamfrisbee’s picture

iamfrisbee commented
Status: Needs work » Needs review

Used the robot to drive myself crazy with pointless white space issues. Some of the formatting I could understand but most of it really did seem pointless. Especially since I my text editor does a good deal of automating that I had to manually remove and not all comments are sentences. But I completed it as expected and this was my brief cry as I go back to being more productive. No wonder WordPress is taking over the CMS market.

bessone’s picture

bessone commented
Status: Needs review » Needs work

Hello,

You should never use t() to translate variables, i found it in youtube_import.module on lines 234 and 580.

You should use a placeholder, some references here: https://api.drupal.org/api/drupal/includes!bootstrap.inc/function/t/7

iamfrisbee’s picture

iamfrisbee commented

Thanks bessone. Found the warning when running that robot, but couldn't figure out what it wanted instead. Figured it was just a warning and let it go. Now I know the secret. And it is fixed.

iamfrisbee’s picture

iamfrisbee commented
Status: Needs work » Needs review
Anatolii88’s picture

Anatolii88 commented

Hi, there are some issues on automated test service.

http://pareview.sh/pareview/httpgitdrupalorgsandboxshaunfrisbee2575089git

It should be easy to fix.

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Yes: Does not cause module duplication and/or fragmentation.
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements.
Coding style & Drupal API usage
List of identified issues:
  1. Just a recommendation: just fix the errors with the spaces in youtube_import.module

If added, please don't remove the security tag, we keep that for statistics and to show examples of security problems.

This review uses the Project Application Review Template.

darol100’s picture

darol100 commented

@Anatolii88,

Should be this on Needs Work or RCTB ?

iamfrisbee’s picture

iamfrisbee commented

This spacing thing is so utterly painful. I thought it would be fun to share something useful with the rest of the Drupal community but I'm starting to regret even trying. So much time wasted on spaces. I understand the importance of white space, but I cannot comprehend the level of importance that is being placed on it. I have fixed the spacing "issue".

jkingsnorth’s picture

jkingsnorth commented
Status: Needs review » Needs work

Hi I've just tried out the module, but I wasn't able to get it to work.

I enter a YouTube username in Content > Youtube import, press save and I receive a number of warnings:
Notice: Trying to get property of non-object in youtube_import_playlist_id() (line 376 of ...\youtube_import\youtube_import.module).

I also wonder whether this module duplicates the functionality already provided by Feeds and the Feeds YouTube Parser module?
https://www.drupal.org/project/feeds_youtube

The above also uses the YouTube API to import videos into nodes - though admittedly this module appears simpler and more focused. Thanks for sharing.

iamfrisbee’s picture

iamfrisbee commented

@JKingsnorth, I used to use the feeds_youtube module. Unfortunately, last I checked anyway, they are still trying to use the old API which is no longer available. I had to create this module because I had clients that were using the feeds_youtube and were no longer getting the updates on their website.

The error you received, though admittedly, I should catch and send something more friendly, I believe is due to an invalid API key. Can you help by verifying your response for:

https://www.googleapis.com/youtube/v3/channels?part=contentDetails&forUs...

The part of the code that is generating the error is trying to get your uploads playlistid in order to display only the videos that you have uploaded. If I understand the API documentation correctly, everyone should have one so my logical guess here is something is wrong with the API key. If you can confirm, I will patch to throw a more useful error.

I have a very limited number of test subjects so it is difficult to work out any bugs that I have not hit myself. Part of the problem with v3 is I'm going to have to document how to create an API key correctly.

jkingsnorth’s picture

jkingsnorth commented

Thanks Shaun, I was being cheeky and trying to get away with not using an API key - that field isn't marked as mandatory and didn't throw an error when I didn't fill it it, so I thought it wasn't required.

iamfrisbee’s picture

iamfrisbee commented

@JKingsnorth. WOW! The issue with testing your own code is that you forget to use it as if you have never seen it before. Made some fields required. I ensured that you entered either a username or a playlist id through the validation hook but completely overlooked all the other required fields. Thanks for pointing that out.

iamfrisbee’s picture

iamfrisbee commented
Status: Needs work » Needs review
iamfrisbee’s picture

iamfrisbee commented

@JKingsnorth so I updated one of the sites I have this running on with the most recent version and it still threw the same error you mentioned. I had an in_array() where I should have had an array_key_exists().

jnavane’s picture

jnavane commented
Priority: Normal » Major
Status: Needs review » Needs work
Issue tags: +code cleanup

Hi,

By working with your module, I have following concerns.

Please look into this.
1. Provide the information of, How / From where the user can get their youtube API key?. May be in README.md file?
2. Provide success/failure notification message after saving the youtube import configuration.
3. When I try to submit the form without playlist ID, I am getting following error.
"Notice: Trying to get property of non-object in youtube_import_playlist_id() (line 379 of C:\xampp\htdocs\drupal-7.39\sites\all\modules\custom\youtube_import\youtube_import.module)."

Please fix these for better user friendly module.

Thanks,
Navaneeth.

iamfrisbee’s picture

iamfrisbee commented
Status: Needs work » Needs review

@Navaneethakrishnan Jayabalan... below are answers to your concerns

1. I linked to the Google Help article in the README.md file for you. Which should also display if you have advanced help installed.
2. I created a success message upon success and added friendly error messages for when Google sends an error back.
3. I was able to recreate this issue only when Google sent an error back. Basically, because Google has their own custom errors and was not using HTTP Response codes, the CURL response was 200 (YAY!!) but the result was not expected. Now, it checks for Google's error message and gives you that instead.

I'm not good with user friendly, but I know I should. Thanks for forcing me.

iamfrisbee’s picture

iamfrisbee commented

I just made an update to set the status value on the node to 1. Realized, on one of my sites, that content was published by default and needed to be for the video import. Is anyone even listening still?

iamfrisbee’s picture

iamfrisbee commented

It's been 2 months without a word. Did I do something wrong?

vikas_jain’s picture

vikas_jain commented

Hi shaunfrisbee,

Please check my few suggestions for you.

1. As your .module file have multiple hooks and functions so it is best practice to Move your admin setting form functionality in .admin.inc

2. Your module works on curl, so please implement "hook_requirements" to check "function_exists('curl_init')".

Thanks.

iamfrisbee’s picture

iamfrisbee commented

vikas_jain,

Thanks for the suggestion on the curl_init. That will definitely be added. Can you explain or point me to the best practice documentation for admin.inc. This being my first attempt at getting one of my custom modules published, there has been a good deal of "the Drupal way" with no real explanation.

Thanks

vikas_jain’s picture

vikas_jain commented

Shaunfrisbee, You can refer following URLs :

https://www.drupal.org/coding-standards

https://www.drupal.org/node/1762288

http://stackoverflow.com/questions/3520928/how-to-split-the-code-of-a-mo...

Thanks.

iamfrisbee’s picture

iamfrisbee commented

vikas_jain, I have been through those a few times but they are still a little vague on how one is expected to split the .module file into pieces. That said, I believe I have accomplished what you were looking for with my last push.

bigmonmulgrew’s picture

bigmonmulgrew commented

What I would love to see for this video is a help page with an embedded youtube video showing how to set it up, just a though. It would make it super slick

iamfrisbee’s picture

iamfrisbee commented

At this rate, I'm probably just going to ditch the work anyway. It was something I created that helped me solve an issue on 4 different sites so I figured it would help others. Now, I feel like I'm doing a lot of work that I really don't need to be doing just trying to share something useful with others.

I like your idea though. I think it would be more suited if I created an entity and themed it to match though. Right now, all this does is pull data from YouTube and put it into your existing content type. It doesn't create any special output or embed anything. Just gives you the data from the API.

I still might consider it as I do like the idea. I just don't have much motivation to put anymore into this as it has already served my purposes.

manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented
Status: Needs review » Needs work

Please add review bonus to boost up the project application review process.

klausi’s picture

klausi
he/him||they/them
Primary language German
Location 🇦🇹 Vienna
commented
Status: Needs work » Needs review

Review bonus is not an application blocker, please do a manual review of the code.

bigmonmulgrew’s picture

bigmonmulgrew commented

I feel your pain. When I first added a project it drove me mad trying to change everything to be Drupal compliant. Even though my code was compliant and making it drupal compliant actially had downsides.

It makes a lot of sense though when you know why, and at the very lease is a little less demoralizing. Drupal coding standards are designed to encourage maximum readability and make things easier to maintain. Drupal is contributed from multiple platforms things that seem silly like 2 spaces instead of a tab help keep things platform netutral.

Those downsides I mentioned were making my css the complete opposite of minified. That drove me mad at first going against all the good habbits I'd picked up for my code.

There are drupal modules however that will minify things for you and reorganise your code to save space when its dealt to the user. Having a platform neutral coding standard helps stuff like that work.

I will say I would find this module immensely useful if you decide you really don't want the hastle then apply for single project promotion and add me as a maintainer to the project. I'll clean up the code for you and help with the module. I don't really know the youtube API well but I do a lot of videos and use youtube a lot so helping you clean up the code is the least I could to.

I've just tested the module with different field settings on my test site. Imported lots of different fields. I can't get it to import into a youtube field. If I import it into a simple text field it imports the URL just fine. I'd consider this the only blocking issue really. The best practice stuff I don't think would be considered blocking, unless a more experienced Drupalite can correct me.

If can show me what field widget I can get it to display a video with I'll consider this an issue with the youtube field widget and consider all your blocking issues cleared up.

I've got a lot of non-blocking stuff to feedback but I'm out of time so I'll check back in tomorrow. Hopefully we can get the blocking issue sorted by then

bigmonmulgrew’s picture

bigmonmulgrew commented
Status: Needs review » Needs work
manjit.singh’s picture

manjit.singh
Primary language Hindi
Location Chandigarh
commented

Automated Review

Found some of the issues at http://pareview.sh/pareview/httpgitdrupalorgsandboxshaunfrisbee2575089git

Manual Review

Individual user account
Yes: Follows the guidelines for individual user accounts.
No duplication
Is there any similarity between your module and https://www.drupal.org/project/feeds_youtube ?
Master Branch
Yes: Follows the guidelines for master branch.
Licensing
Yes: Follows the licensing requirements.
3rd party assets/code
Yes: Follows the guidelines for 3rd party assets/code.
README.txt/README.md
Yes: Follows the guidelines for in-project documentation and/or the README Template.
Code long/complex enough for review
Yes: Follows the guidelines for project length and complexity.
Secure code
Yes: Meets the security requirements
Coding style
Is it possible to use the proper naming conventions for variables in youtube_import.module file. I have seen lots of variable name with single alphabet letter that is bit complicated to understand.
bigmonmulgrew’s picture

bigmonmulgrew commented

So Just to create a little summary or whats been said already and list the non-blocking issues I didn't get time to write out already.

  1. [*] Share video URL not imported into field for youtube field.
  2. Imported content says it was creted by Anonymous User- Would be nice to set this manually or have it as the user who runs the import
  3. Move admin settings to an admin.inc to keep them seperate
  4. Change variable names to be a little more descriptive
  5. Correct whitespace issues
iamfrisbee’s picture

iamfrisbee commented

bigmonmulgrew, thanks for the validation. It is nice to know that I'm not the only one frustrated by this.

I have never heard of a YouTube field but some Google gave me this: https://www.drupal.org/project/youtube. It shouldn't take much for me to add that in. So that's item #1. Number 2 should be fairly easy as well. Number 3 is already done. Number 4... ugh... okay. I can see the value in that someone else is trying to read my code. Number 5... I guess I need to run the robot again. I thought I had already fixed all of those.

As far as you being a maintainer... I am pretty sure I added you. I have no doubt that you could make or suggest a great deal of improvements.

Thanks. Really. I appreciate this.

iamfrisbee’s picture

iamfrisbee commented

bigmonmulgrew, I have added the YouTube field functionality.

bigmonmulgrew’s picture

bigmonmulgrew commented
Status: Needs work » Reviewed & tested by the community

Facepalm. I already knew 3 was done. I had been looking through the file. I must have forgotten when I wrote that.

How were you displaying the videos on the site if you were not using youtube field. I tried media field too and another with escapes me at the moment. They all had the same issue.

Thanks for adding youtube field I appreciate that. Still curious how you were doing things before though.

I think we can forgive these issues of coding standard as non-blocking. It would be nice if you can get them done but I'm changin this to reviewed and tested. We can address the minor issues after promotion.

I'm going to refrain from making any cleanups of the code until you decide to either ask for single project promotion of it gets promoted to a full project since this is part of you gettting git vetted

iamfrisbee’s picture

iamfrisbee commented

And now I have added the Author field you requested.

To display the videos on my current sites, I just used the URL and changed the field output through the theme. It is not the most user friendly module, but I thought it would help another developer keep from reinventing the wheel. Also, the two sites were using another module that no longer works so they already had an existing content type and themed output. I just needed to fill in the blanks.

I will now go and figure out how to make this a single project as I have no idea what that means.

bigmonmulgrew’s picture

bigmonmulgrew commented

I just did a little test by tweaking the readme file and I cant push commits. I'm not 100% sure but I think thats deliberate on sandbox projects???? Not sure though. Either that or the permissions are not set.

Anyway lets not worry about that for now. Do as much as you caan to clean up the minor issues and hope the git admins agree we can forgive them and let you promote.

The bot can be a pain in the ass I swear on my sanbox I have at least 30 commits that are just cleanign up code to keepp the bot happy.

bigmonmulgrew’s picture

bigmonmulgrew commented

Single project promotion is you asking the Git Admins to promote the project for you rather than giving you permission to promote projects yourself. Its usually user for projects too short to make a decision on if the maintainer knows what they are doing.

Really theres no need for you to ask for single project promotion any more since in my opinion this should be enough to grant you the ability to promote yourself. Take that opinion with a pinch of salt as I don't know all the details of the requirements.

iamfrisbee’s picture

iamfrisbee commented

bigmonmulgrew, I have updated everything to match the fun and excitement in your 5 suggestions. We have friendly variable names, the review robot is happy, things have been moved to the proper files. I think it is ready. The only incomplete suggestion was the help video suggestion that I cannot find any more. The video will have to wait. I still don't have a promote option though so I guess we are waiting for now. Thanks for all the help. I'll be sure to add you as a maintainer if this ever gets promoted.

bigmonmulgrew’s picture

bigmonmulgrew commented
Status: Reviewed & tested by the community » Needs review
bigmonmulgrew’s picture

bigmonmulgrew commented
Status: Needs review » Reviewed & tested by the community

Fantastic. Just for the sake of completeness i have just retested the module on my test site and everythign works beautifully.

Automated review is currently showing no errors. I've been through the code and its now a pleasure to read. The bot is happy (which is the most important part, we don't want it revolting)

Now if I understand the process correctly all that is left is for the git admins to take a look and if they are happy they wil give you permission to promote the project yourself. You wont get that until the overlords look over it though, so yes now we just wait

iamfrisbee’s picture

iamfrisbee commented
Issue tags: -code cleanup

Removed the Issue Tag for "code cleanup". Not sure where that came from.

iamfrisbee’s picture

iamfrisbee commented

I have added some functionality to automatically include YouTube fields if they exist and to allow for YouTube fields to be checked for an existing video so that a video is not imported twice. So that should be cool. I checked with the robots again and they still like me.

PA robot’s picture

PA robot commented
Status: Reviewed & tested by the community » Closed (duplicate)
Multiple Applications
It appears that there have been multiple project applications opened under your username:

Project 1: https://www.drupal.org/node/2696331

Project 2: https://www.drupal.org/node/2575109

As successful completion of the project application process results in the applicant being granted the 'Create Full Projects' permission, there is no need to take multiple applications through the process. Once the first application has been successfully approved, then the applicant can promote other projects without review. Because of this, posting multiple applications is not necessary, and results in additional workload for reviewers ... which in turn results in longer wait times for everyone in the queue. With this in mind, your secondary applications have been marked as 'closed(duplicate)', with only one application left open (chosen at random).

If you prefer that we proceed through this review process with a different application than the one which was left open, then feel free to close the 'open' application as a duplicate, and re-open one of the project applications which had been closed.

I'm a robot and this is an automated message from Project Applications Scraper.

iamfrisbee’s picture

iamfrisbee commented
Status: Closed (duplicate) » Reviewed & tested by the community

I closed the other project as this is the one that has gone further through the review process. I didn't realize how this works. I assume that I once did as I have 3 sandbox projects and only one going through this process, but I must have forgotten.

mlncn’s picture

mlncn commented
Status: Reviewed & tested by the community » Fixed

Thanks for sticking with this, and thanks for your contribution! Congratulations, you are now a vetted Git user. You can promote this to a full project.

When you create new projects (typically as a sandbox to start) you can then promote them to a full project.

Here are some recommended readings to help with excellent maintainership:

You can find lots more contributors chatting on IRC in #drupal-contribute. So, come hang out and stay involved!

Thanks, also, for your patience with the review process. Anyone is welcome to participate in the review process. Please consider reviewing other projects that are pending review. I encourage you to learn more about that process and join the group of reviewers.

Status: Fixed » Closed (fixed)

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