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

Switch to full object links DB #445

Closed
ArthurHoaro opened this issue Jan 20, 2016 · 6 comments · Fixed by #1307
Closed

Switch to full object links DB #445

ArthurHoaro opened this issue Jan 20, 2016 · 6 comments · Fixed by #1307

Comments

@ArthurHoaro
Copy link
Member

Disclaimer: this is technical.

LinkDB currently implements the ArrayAccess interface. It means that we have an object which behave like an array and works fine with our datastore file. Brilliant idea! In theory.

From SO:

When you accessing elements from using the [] operator it behaves not exactly like an array. Internally it's offsetGet() method is called, and will return in your case an array - but not a reference to that array.

This leads to my issue because our data structure is a multidimensional array. I encountered issues setting data in this array and, most importantly, I couldn't run some array_* functions. So it kinda loses its initial goal.

I think it would be better to have a LinkDB class which manages Link objects. And then only our read/writeDB() functions handle the object <-> array <-> file interface.

@virtualtam
Copy link
Member

A dedicated Link object could also handle common processing that is usually applied before rendering templates:

  • date formatting
  • tag list formatting
  • whether a redirector is used
  • ...

@ArthurHoaro
Copy link
Member Author

ArthurHoaro commented Apr 7, 2017

I'm starting to have a viable solution in early stage of development which would let us work on multiple open issues more easily due to the following changes:

  • how the datastore is accessed: access private links without be logged in, handle special tags (nomarkdown)
  • how the links are stored (the actual point of this issue): a lot of duplicated code in every controller would be factorized (as virtualtam mentioned)
  • how the links are rendered: move the markdown plugin to the core, be able to highlight search results, add other renderer, etc.

However, all of this requires breaking backward compatibility changes. I don't see any way to do this using the Updater because previous update methods/functions touching the data store won't work any more (except, maybe, by keeping a lot of dead code).

This means that users willing to update would have to either (let's assume that's for v0.10):

  • upgrade to the latest v0.9, then to the latest version
  • export their links, update, import them back.

IMHO, this needs to be done before switching to Slim controllers, and without it Shaarli's refactoring won't go a lot further.

@virtualtam @nodiscc Do you think this acceptable?

@virtualtam
Copy link
Member

👍
Could you push the current changes (no matter how rough the state) on a branch so we have an overview of the underlying code changes?

Regarding backwards compatibility, we could keep the current datastore loader as a LegacyLinkDB class providing:

  • datastore reading
  • migration to the new data format

This might require to also have a LegacyUpdater class, to provide a workflow in the lines of:

  • access Shaarli
  • detect datastore format
    • up-to-date: N/A
    • up-to-date format, data updates needed: run Updater
    • legacy format:
      • run LegacyUpdater
      • load data with LegacyLinkDB, save in the new format
      • run Updater

@ArthurHoaro
Copy link
Member Author

I've pushed the current state in a609ded. It's really an early stage, and not really working yet, but you'll get the idea:

  • Link represents a link, and carry a bit a basic logic such as URL protocol, dependency between fields, etc.
  • LinksIO handles file operation.
  • LinksArray supports ArrayAccess operations of current LinkDB, only for internal use.
  • The entry point for all controllers is LinksService which wraps any access to the links. It relies a lot on the current LinksFilter.
  • LinkFormatter transforms a Link object to an array for the templates (I'm still not sure where it should intervene).

Your update suggestion should work, I'll try to do something like this.

@nodiscc
Copy link
Member

nodiscc commented Apr 13, 2017

This is interesting, from what I understand your original problems are:

ArrayAccess will return in an array - but not a reference to that array.

I couldn't run some array_* functions.
Prevents fully switching to Slim controllers

a lot of duplicated code in every controller

Right?

requires breaking backward compatibility
let's assume that's for v0.10
export their links, update, import them back
LegacyLinkDb, LegacyUpdater classes

(If there are non-backward-compatible changes, we should bump the major version number (eg 1.x) as per https://github.com/shaarli/Shaarli/blob/master/CHANGELOG.md -> http://semver.org/)

@ArthurHoaro Moving the current code to LegacyLinkDb, then implementing the new LinksService you proposed in a609ded would be a good way to proceed I think. 👍

Having a LegacyUpdater class would be great for automated migration of the legacy datastore, though a manual export/import/upgrade step would be acceptable. At the very least, the legacy datastore should be detected after upgrade, then a short notice should be displayed to the user (export/reimport procedure explanation). On successful import the old datastore could be discarded (extra caution to data loss).

Can you explain shortly how it would change things regarding:

access private links without be logged in, handle special tags (nomarkdown), move the markdown plugin to the core, be able to highlight search results, add other renderer

Thanks a lot

@ArthurHoaro
Copy link
Member Author

ArthurHoaro commented Apr 13, 2017

Yes, those were my original problems. But also, I've tried to work on a few issues I mentioned, and the current LinkDB needs to be refactored if we don't want to end up with a class handling too much logic (and again too many lines, a lot of if/else, etc.). So I'd like to do it in a proper way.

access private links without be logged in

LinkService would act as a very basic entrypoint like you would use an ORM. The entire datastore would be loaded, and then links would be retrieved using for example findAll(visibility) or any filter we want. The controllers then would have to load the proper link list they need. Currently, we already filter and format the links when they're read from the datastore.

handle special tags (nomarkdown), move the markdown plugin to the core, be able to highlight search results, add other renderer

Currently, links are mostly formatted in controllers using functions, then these functions are rolled back in the markdown plugin to apply markdown rendering (e.g. add <a> tag on URLs). I want to add a formatter class which would convert the Link object into a displayable array. Depending on the formatter chosen (DefaultFormatter or MarkdownFormatter), links array would be different. With this, controllers would only have to call a format($link) method.

I still have to figure out how to make LinkFilter interact with formatters to highlight search results, though.

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

Successfully merging a pull request may close this issue.

3 participants