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

Add check for max length of strings, reduce classes size #251

Merged
merged 1 commit into from Jan 5, 2015
Merged

Add check for max length of strings, reduce classes size #251

merged 1 commit into from Jan 5, 2015

Conversation

flodolo
Copy link
Collaborator

@flodolo flodolo commented Dec 22, 2014

LangManager

  • Extract MAX_LENGTH from lang files
  • Move ['python_vars'] to ['errors']['python'] to support more types of errors
  • Add function countErrors
  • Move functions for .po files into a new class
  • Move error checks in separate functions (python, length)

Utils

  • starsWith: accept array of strings as needle
  • Use mb_strlen instead of strlen, add getLength

GetTextManager

  • Functions to manage .po files originally in LangManager

Views
Expose number of errors in JSON, display errors in listsitesperlocale and errors view

Clean up phpDOC syntax in all classes.

@flodolo
Copy link
Collaborator Author

flodolo commented Dec 22, 2014

The diff is huge mostly because of the phpDOC cleanup and code movements. Some notes to make review a little easier:

  • I copied the implementation that we're using on Transvision for Utils::StartsWith, accepting an array of needles instead of a single string (copied also the tests).
  • The code in GetTextManager hasn't changed, it just moved from the old class. Same goes for tests.
  • the checkPythonErrors code is the same, just moved in its own function

@flodolo
Copy link
Collaborator Author

flodolo commented Dec 31, 2014

Regarding this bug ("attach to bugzilla" not selecting the locale component): it's actually true, and I don't think it ever worked after the initial refactor (I tracked back the current link, which used to be called $bugzilla, to the initial commit).

To fix it I'm including the Bugzilla class from Webdashboard, so that we can create locale names like "id / Indonesian". Not sure if there are better ways to do this, since this forces us to keep the class in sync between projects. Also kept this change in its own commit for now.

@pascalchevrel
Copy link
Contributor

Maybe we should maintain a separate repo for the libs we use across projects and include them via composer, I checked Composer documentation and it seems possible to include packages from repositories not registered in Packagist.

@flodolo
Copy link
Collaborator Author

flodolo commented Jan 5, 2015

Definitely worth it (probably not in this PR). We now have a Bugzilla class across langchecker+webdashboard, we could probably build a class for generic string related functions, we already have Json (in completely different statuses) in Transvision+langchecker+webdashboard.

@flodolo
Copy link
Collaborator Author

flodolo commented Jan 5, 2015

Did some tests and it's indeed not too difficult to do, but I think it's easier to discuss it in a different PR
https://github.com/flodolo/php-bugzilla-class

@pascalchevrel
Copy link
Contributor

Tested locally, works fine, r+ with nits

**LangManager**
* Extract MAX_LENGTH from lang files
* Move ['python_vars'] to ['errors']['python'] to support more types of errors
* Add function countErrors
* Move functions for .po files into a new class
* Move error checks in separate functions (python, length)

**Utils**
* starsWith: accept array of strings as needle
* Use mb_strlen instead of strlen, add getLength

**GetTextManager**
* Functions to manage .po files originally in LangManager

**Views**
Expose number of errors in JSON, display errors in listsitesperlocale and errors view

**Bugzilla**
* Include Bugzilla class from Webdashboard and fix 'attach to this bug issue'. Add basic testing.

Clean up phpDOC syntax in classes.
@flodolo
Copy link
Collaborator Author

flodolo commented Jan 5, 2015

Thanks. Fixed nits and squashed, I'll wait for the webdashboard's one to merge.

@flodolo
Copy link
Collaborator Author

flodolo commented Jan 5, 2015

Actually leaving the merge to you since this requires a php composer.phar update on the server to avoid exploding.

pascalchevrel added a commit that referenced this pull request Jan 5, 2015
Add check for max length of strings, reduce classes size
@pascalchevrel pascalchevrel merged commit aef4af9 into mozilla-l10n:master Jan 5, 2015
@flodolo flodolo deleted the length_limit branch January 5, 2015 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants