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

Refactor bookmark import using a generic Netscape parser #612

Merged
merged 1 commit into from
Aug 10, 2016

Conversation

virtualtam
Copy link
Member

@virtualtam virtualtam commented Jul 28, 2016

Relates to #607
Relates to #608
Relates to #493 (abandoned)

Additions:

  • use Composer's autoload to load 3rd-party dependencies under vendor/

Modifications:

  • [import] replace the current parser with a generic, stable parser
    • move code to application/NetscapeBookmarkUtils
    • improve status report after parsing
  • [router] use the same endpoint for both bookmark upload and import dialog
  • [template] update bookmark import options
  • [tests] ensure bookmarks are properly parsed and imported in the LinkDB
    • reuse reference input from the parser's test data

See:

@virtualtam virtualtam added enhancement in progress cleanup code cleanup and refactoring tools developer tools template HTML rendering 3rd party interoperability with third-party platforms labels Jul 28, 2016
@virtualtam virtualtam added this to the 0.8.0 milestone Jul 28, 2016
@virtualtam virtualtam self-assigned this Jul 28, 2016
@virtualtam virtualtam force-pushed the refactor/bookmark-parser branch from 35dae13 to ce3b210 Compare July 28, 2016 22:00
@virtualtam virtualtam force-pushed the refactor/bookmark-parser branch from ce3b210 to a2516fa Compare July 28, 2016 22:10
@virtualtam virtualtam force-pushed the refactor/bookmark-parser branch 2 times, most recently from a6b7fd5 to 3d9e478 Compare July 30, 2016 15:43
@virtualtam virtualtam changed the title [WIP]Refactor bookmark import using a generic Netscape parser Refactor bookmark import using a generic Netscape parser Jul 30, 2016
if (empty($post['default_tags'])) {
$defaultTags = array();
} else {
$defaultTags = preg_split('/[\s,]+/', $post['default_tags']);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The user input should be escaped.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should input sanitizing be (optionally) held by the parser, or be left to client code?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be done by the client. The parser shouldn't know if it needs to be escaped or not (ex: let's say that default tags are retrieve through an ORM which returns escaped data).

@ArthurHoaro
Copy link
Member

ArthurHoaro commented Aug 1, 2016

There is a bug somewhere. I made an import with a similar database, with overwrite and import as private checked. The import ran fine, but my previous private links are now public (and the public ones are private).

EDIT: aside from that and my comment, I'm OK the PR. Nice work with the parser BTW!

@virtualtam
Copy link
Member Author

Yup, still a bit rough around the edges, I still have to add UTs and do more testing :)

@virtualtam virtualtam force-pushed the refactor/bookmark-parser branch 7 times, most recently from f00379f to a648e28 Compare August 7, 2016 22:37
/**
* The user-specified tags contain characters to be escaped
*/
public function testSanitizeDefaultTags()
Copy link
Member Author

@virtualtam virtualtam Aug 7, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure for this one; there are various ways of sanitizing inputs & outputs throughout the code. I think the cleanup should be similar to when a new link is added, which corresponds to the following section:

Apart from tag cleanup, there doesn't seem to be a lot of HTML / special char escaping...

Maybe these operations should be handled by LinkDB to provide a more consistent behaviour?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, and I made a mistake on this. We actually sanitize link fields when the DB is retrieved, not when links are added/edited. See https://github.com/shaarli/Shaarli/blob/master/application/LinkDB.php#L290.

I remember doing that, but that doesn't feel like a good idea now. We might want to change that later, maybe in #445. Anyway, it means that you don't have to worry about link fields sanitization here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, thanks for the clues ;-)

@virtualtam
Copy link
Member Author

Comments taken into account + test code updated to run with PHP 5.3:

  • fix MAX_FILE_SIZE position in the HTML upload form
  • add missing @return statements
  • redirect to the import page after uploading a file, whatever the import result is
  • keep input sanitizing to the bare minimum
  • do not use array dereferencing in test code - this harms readability a bit, the concerned tests are quite straightforward though

virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Aug 9, 2016
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli/netscape-bookmark-parser#15

Modification:
- reference the "shaarli" vendor repository on Packagist instead of
  overriding the upstream package with an SCM repository

See https://packagist.org/packages/shaarli/netscape-bookmark-parser

Signed-off-by: VirtualTam <[email protected]>
@virtualtam virtualtam force-pushed the refactor/bookmark-parser branch from babd7b5 to efed72f Compare August 9, 2016 23:02
Relates to shaarli#607
Relates to shaarli#608
Relates to shaarli#493 (abandoned)

Additions:
- use Composer's autoload to load 3rd-party dependencies under vendor/

Modifications:
- [import] replace the current parser with a generic, stable parser
  - move code to application/NetscapeBookmarkUtils
  - improve status report after parsing
- [router] use the same endpoint for both bookmark upload and import dialog
- [template] update bookmark import options
  - allow adding tags to all imported links
  - allow selecting the visibility (privacy) of imported links
- [tests] ensure bookmarks are properly parsed and imported in the LinkDB
  - reuse reference input from the parser's test data

See:
- https://github.com/shaarli/netscape-bookmark-parser
- https://getcomposer.org/doc/01-basic-usage.md#autoloading

Signed-off-by: VirtualTam <[email protected]>
@virtualtam virtualtam force-pushed the refactor/bookmark-parser branch from efed72f to a973afe Compare August 9, 2016 23:42
@virtualtam
Copy link
Member Author

@ArthurHoaro ArthurHoaro merged commit e0dd77c into shaarli:master Aug 10, 2016
@virtualtam
Copy link
Member Author

w00t! thanks @ArthurHoaro for the merge :)

@virtualtam virtualtam deleted the refactor/bookmark-parser branch August 10, 2016 11:50
@ArthurHoaro
Copy link
Member

No problem, thanks for your work on this! =)

virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Aug 12, 2016
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli#624

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Additions:
- [all][env] set $TERM=dumb to avoid debconf-related issues
- [all][pkg] install ca-certificates
- [all][pkg] cleanup APT cache after installing packages
- [dev] refactor Dockerfile

TODO:
- [prod][php] use Composer to resolve PHP dependencies
- [prod] refactor Dockerfile

Signed-off-by: VirtualTam <[email protected]>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Aug 12, 2016
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli#624

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Modifications:
- [all][env] set $TERM=dumb to avoid debconf-related issues
- [all][pkg] install ca-certificates
- [all][pkg] cleanup APT cache after installing packages
- [dev] refactor Dockerfile
- [prod][master][php] use Composer to resolve PHP dependencies
- [prod][master] refactor Dockerfile

TODO:
- [prod][stable][php] use Composer to resolve PHP dependencies
- [prod][stable] refactor Dockerfile

Signed-off-by: VirtualTam <[email protected]>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Aug 12, 2016
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli#624

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Modifications:
- [all][env] set $TERM=dumb to avoid debconf-related issues
- [all][pkg] install ca-certificates
- [all][pkg] cleanup APT cache after installing packages
- [dev] refactor Dockerfile
- [prod][master][php] use Composer to resolve PHP dependencies
- [prod][master] refactor Dockerfile

Commented modifications:
- [prod][stable][php] use Composer to resolve PHP dependencies
- [prod][stable] refactor Dockerfile

Signed-off-by: VirtualTam <[email protected]>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Aug 12, 2016
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli#624

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Modifications:
- [all][env] set $TERM=dumb to avoid debconf-related issues
- [all][pkg] install ca-certificates
- [all][pkg] cleanup APT cache after installing packages
- [dev] refactor Dockerfile
- [prod][master][php] use Composer to resolve PHP dependencies
- [prod][master] refactor Dockerfile
- [prod][stable] refactor Dockerfile

Commented modifications:
- [prod][stable][php] use Composer to resolve PHP dependencies

Signed-off-by: VirtualTam <[email protected]>
@nodiscc
Copy link
Member

nodiscc commented Aug 14, 2016

Thank you all

virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Aug 14, 2016
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli#624

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Modifications:
- [all][env] set $TERM=dumb to avoid debconf-related issues
- [all][pkg] install ca-certificates
- [all][pkg] cleanup APT cache after installing packages
- [dev] refactor Dockerfile
- [prod][master] refactor Dockerfile
- [prod][master][php] use Composer to resolve PHP dependencies

Signed-off-by: VirtualTam <[email protected]>
virtualtam added a commit to virtualtam/Shaarli that referenced this pull request Aug 14, 2016
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli#624
Relates to shaarli#633

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Modifications:
- [prod][stable] refactor Dockerfile
- [prod][stable] set $TERM=dumb to avoid debconf-related issues
- [prod][stable] install ca-certificates
- [prod][stable] cleanup APT cache after installing packages
- [prod][stable] use Composer to resolve PHP dependencies

Signed-off-by: VirtualTam <[email protected]>
ArthurHoaro pushed a commit that referenced this pull request Nov 5, 2016
Relates to #607
Relates to #612
Relates to shaarli/netscape-bookmark-parser#15

Modification:
- reference the "shaarli" vendor repository on Packagist instead of
  overriding the upstream package with an SCM repository

See https://packagist.org/packages/shaarli/netscape-bookmark-parser

Signed-off-by: VirtualTam <[email protected]>
ArthurHoaro pushed a commit that referenced this pull request Nov 5, 2016
Relates to #607
Relates to #612
Relates to #624

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Modifications:
- [all][env] set $TERM=dumb to avoid debconf-related issues
- [all][pkg] install ca-certificates
- [all][pkg] cleanup APT cache after installing packages
- [dev] refactor Dockerfile
- [prod][master] refactor Dockerfile
- [prod][master][php] use Composer to resolve PHP dependencies

Signed-off-by: VirtualTam <[email protected]>
ArthurHoaro pushed a commit that referenced this pull request Nov 5, 2016
Relates to #607
Relates to #612
Relates to #624
Relates to #633

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Modifications:
- [prod][stable] refactor Dockerfile
- [prod][stable] set $TERM=dumb to avoid debconf-related issues
- [prod][stable] install ca-certificates
- [prod][stable] cleanup APT cache after installing packages
- [prod][stable] use Composer to resolve PHP dependencies

Signed-off-by: VirtualTam <[email protected]>
portailp pushed a commit to PortailPro/Shaarli that referenced this pull request Mar 20, 2017
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli/netscape-bookmark-parser#15

Modification:
- reference the "shaarli" vendor repository on Packagist instead of
  overriding the upstream package with an SCM repository

See https://packagist.org/packages/shaarli/netscape-bookmark-parser

Signed-off-by: VirtualTam <[email protected]>
portailp pushed a commit to PortailPro/Shaarli that referenced this pull request Mar 20, 2017
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli#624

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Modifications:
- [all][env] set $TERM=dumb to avoid debconf-related issues
- [all][pkg] install ca-certificates
- [all][pkg] cleanup APT cache after installing packages
- [dev] refactor Dockerfile
- [prod][master] refactor Dockerfile
- [prod][master][php] use Composer to resolve PHP dependencies

Signed-off-by: VirtualTam <[email protected]>
portailp pushed a commit to PortailPro/Shaarli that referenced this pull request Mar 20, 2017
Relates to shaarli#607
Relates to shaarli#612
Relates to shaarli#624
Relates to shaarli#633

See https://github.com/shaarli/Shaarli/wiki/Server-requirements

Modifications:
- [prod][stable] refactor Dockerfile
- [prod][stable] set $TERM=dumb to avoid debconf-related issues
- [prod][stable] install ca-certificates
- [prod][stable] cleanup APT cache after installing packages
- [prod][stable] use Composer to resolve PHP dependencies

Signed-off-by: VirtualTam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3rd party interoperability with third-party platforms cleanup code cleanup and refactoring enhancement template HTML rendering tools developer tools
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants