Skip to content
This repository was archived by the owner on May 1, 2019. It is now read-only.

[WIP] [RFC] Implement Packagist-style module add process #352

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

adamlundrigan
Copy link
Contributor

Overview

Drop the current module add process (n+1 GitHub API requests to fetch all the repositories for your account and determine which are ZF2 modules) in favour of something closer to what Packagist does:

  1. Enter the URL to your repository
  2. In an AJAX request, pull the repository and determine if it's a ZF2 module
    • If it is a module, import it and return the new module's details + link to it's view page
    • If it isn't a module (or if something fails), return a suitable error message
  3. If a module was successfully created, add a row for it below the add form so the user can see it has been added and either open it's view page in a new window or continue adding more modules.

There are two main upsides to this approach

  • Uses far fewer GitHub API requests, which is important as the Search API used to determine if a repository contains a ZF2 module is limited to 30 requests per minute.
  • Makes supporting multiple sources (eg: bitbucket) easier.

TODO list

TODO: needs more TODOs

  • Implement UI for triggering module add
  • Implement background handling of add requests
    • Packagist package names
    • GitHub repository slug

@Ocramius
Copy link
Member

Ocramius commented Feb 1, 2015

In an AJAX request, pull the repository and determine if it's a ZF2 module

Uhhh.... userland validation? :-\

the Search API used to determine if a repository contains a ZF2 module is limited to 30 requests per minute.

That's not really terrible from an API PoV: how many submissions/minute do we even hope to get?

@adamlundrigan
Copy link
Contributor Author

Uhhh.... userland validation? :-\

That's not really terrible from an API PoV: how many submissions/minute do we even hope to get?

Not userland validation, bad wording on my part. Right now the site makes one call to the Search API for each repository in your GitHub account, so if you have more than 30 repositories you bump against the limit. What I propose is nearly identical to the method the site currently uses except it's executed on just one repository (the one you specify the URL for) and not for every repository on your account at once.

That also cures the 30-request-per-minute issue as the likelihood of being able to submit 30 modules to the site in under a minute is pretty slim IMO, so it becomes a non-issue. Even if you do manage that extraordinary feat we can display a message saying "You've reached the rate limit, please wait X seconds and try again"

@localheinz
Copy link
Member

Not sure, but would this be related to #84?

@adamlundrigan
Copy link
Contributor Author

Yes, that is pretty much what I had planned for the client side behaviour.

@localheinz
Copy link
Member

@adamlundrigan

Cool! 👍

@adamlundrigan
Copy link
Contributor Author

It's been a challenge locking down time to work on it, but I still hope to have a prototype out for feedback tomorrow.

@adamlundrigan adamlundrigan force-pushed the packagist-style-module-add branch from 39b3092 to 8b6e4a6 Compare February 4, 2015 01:16
@adamlundrigan
Copy link
Contributor Author

Quick summary of the major work to date:

  • Moved existing login/logout functionality to /auth so I could introduce a new controller to handle rendering the user module listing page (4cd2096)
  • Moved rendering of the module list to it's own controller action - commit has more details on the why (ec27d0f)
  • ZfModule\Mapper\Module::findByOwner doesn't actually filter by owner. Now it does when $owner is not empty. There aren't any tests for the mapper, but there should be...

@localheinz
Copy link
Member

@adamlundrigan @ins0

Please let me know if I can help you guys - I don't want to make it extra-hard for you with me working on a bunch of things, leaving your PRs unmergable and whatnot.

@adamlundrigan
Copy link
Contributor Author

@localheinz no worries, there's not much in here yet. I've been yanked away to deal with some time-sensitive client work so I haven't had time recently to work on it. It'll probably be late next month before I can get back to it, so if anyone else wants to pick it up in the meantime feel free...I can add them as a collaborator on my repo or they can start a new PR.

@samsonasik
Copy link
Contributor

it need rebase

@gianarb
Copy link
Contributor

gianarb commented Oct 18, 2015

News? :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants