Include massive BC breaking changes into Shaarli repository or maintain my own fork ?

[TangoMan75]

Hi Arthur,

I recently happen to need a php parser for a bookmark file and stumped upon Shaarli's repository. I cleaned up as much code as I could without introducing any backward compatibility breaks so that Shaarli community can benefit from improvements as well.

But, I plan to make the parser more compatible and easier to use in my Symfony project, and I'm afraid this will introduce a lot of BC breaking changes.

Would you be willing to take hold of the significant workload needed for a hypothetical 4.0 integration into Shaarli project, or do you believe I should create and maintain my own fork ?

Here are the changes I am planning to introduce:

• Rename class to "NetscapeBookmarkDecoder" for nomenclature consistency.
• 'parseBoolean' method should always return a boolean value / refactor tests accordingly.
• All methods from parser should be private except for entrypoint / remove tests accordingly.
• Remove dependency on Katzgrau\KLogger and remove logging altogether.
• Remove useless "parseFile" method which violates Single Responsibility Principle / update documentation accordingly.
• Replace parameters from constructor with "$context" array and class constants: This will drastically change class signature but will allow for Dependency Injection and Symfony autowiring.

And finally:

• Make parser implement "Symfony\Component\Serializer\Encoder\DecoderInterface". I understand this change would create an unwanted dependency on Symfony Serializer but I am willing to make a version for Shaarli if necessary.
• Rename "parseString" method to "decode" for nomenclature consistency and Inferface conformity.
• Create "supportsDecoding" method for Inferface conformity and Symfony services integration.

New features (maybe):

• Use Data Transer Object design pattern for bookmark.
• Include "ICON" property.
• Create "NetscapeBookmarkEncoder" to return bookmark file from array.
• Rename properties to be (mostly) compatible with "https://schema.org" structured data vocabulary:
  • 'uri' => 'url'
  • 'title' => 'name'
  • 'icon' => 'image'
  • 'note' => 'description'
  • 'tags' => 'tags'
  • 'time' => 'dateCreated'
  • 'pub' => 'published'

[ArthurHoaro]

Hi!

There are a lot of interesting changes in there. I think that it would obviously benefit everyone to include these changes here instead of having another fork. It might however take longer as I might take a few days to review and include your changes.

I don't mind upgrading Shaarli's integration to include the BC breaks, as it shouldn't take too long. We should however write a small upgrade documentation as this library might be used elsewhere.

Regarding your suggested changes I agree with most of them, but I have a few comments:

All methods from parser should be private except for entrypoint / remove tests accordingly.

Good point, but rather protected to avoid preventing class extension.

Remove dependency on Katzgrau\KLogger and remove logging altogether.

What do you think about injecting an optional Psr\Log\LoggerInterface instead? It would also work well with Symfony DI.

Replace parameters from constructor with "$context" array and class constants

These options should be properly documented in the readme as it's usually less easy to use than named parameters.

Make parser implement "Symfony\Component\Serializer\Encoder\DecoderInterface". I understand this change would create an unwanted dependency on Symfony Serializer but I am willing to make a version for Shaarli if necessary.

I'd rather avoid this dependency here. I've already had issues in the past with libraries tied with Laravel components while using Symfony.

Use Data Transer Object design pattern for bookmark.

Do you mean returning a list of objects instead of arrays? That might be easier to use, and wouldn't need to rename array properties. This one should be decided before releasing a new major version.

---

ping @nodiscc

[TangoMan75]

Hello again Arthur,

We should however write a small upgrade documentation as this library might be used elsewhere.

Sure.

Good point, but rather protected to avoid preventing class extension.

I don't mind ; wouldn't this mean a violation of Open / Close principle though ? Would children objects actually need to use parseDate, normalizeDate, parseBoolean, sanitizeString, splitTagString, sanitizeTags or flattenTagsList methods ? These seem to implement very specific business logic to me.

What do you think about injecting an optional Psr\Log\LoggerInterface instead? It would also work well with Symfony DI.

Hmmm... I'm not happy about this idea. Logger is too verbose in the first place IMHO ; And even for debugging purposes this log is not helping much... Nonetheless Symfony will automatically inject default logger which will consume resources, slowing parser down significantly.

These options should be properly documented in the readme as it's usually less easy to use than named parameters.

Of course ; Note this change is absolutely necessary in order to have proper Symfony compatibility.

Here's what I have in mind:

    // ...

    public const KEEP_NESTED_TAGS    = 'keep_nested_tags';
    public const DEFAULT_TAGS        = 'default_tags';
    public const DEFAULT_PUBLICATION = 'default_publication';
    public const NORMALIZE_DATES     = 'normalize_dates';
    public const DATE_RANGE          = 'date_range';

    private $defaultContext = [
        self::KEEP_NESTED_TAGS    => true,
        self::DEFAULT_TAGS        => [],
        self::DEFAULT_PUBLICATION => false,
        self::NORMALIZE_DATES     => true,
        self::DATE_RANGE          => '30 years',
    ];

    public function __construct(array $defaultContext = [])
    {
        $this->defaultContext = array_merge($this->defaultContext, $defaultContext);
    }

I'd rather avoid this dependency here. I've already had issues in the past with libraries tied with Laravel components while using Symfony.

I understand... I believe I can find a way around this.

Do you mean returning a list of objects instead of arrays?

No, sorry... I changed my mind. I realize now that would be a violation of the Single Responsibility Principle. A proper Denormalizer should implement this behavior.

BTW, I have noticed that tags can own attributes like ADD_DATE and LAST_MODIFIED in some bookmark files. Having nested arrays representing both bookmark and tag would allow to map this data correctly.

$bookmark = [
  'name' => 'foobar',
  //...
  'tags' => [
    [
      'name' => 'foo',
      'dateCreated'  => '000000000',
      'dateModified' => '000000000'
    ],
    [
      'name' => 'bar',
      //...
    ],
  ]
]

wouldn't need to rename array properties.

Having array properties renamed according to schema.org vocabulary will have the added benefit to integrate nicely with modern API standards, automatic documentation and improved SEO : https://api-platform.com/docs/admin/schema.org/#using-the-schemaorg-vocabulary

[ArthurHoaro]

I don't mind ; wouldn't this mean a violation of Open / Close principle though ?

Hm I suppose it is. I don't really see a benefit of locking it though.

Hmmm... I'm not happy about this idea. Logger is too verbose in the first place IMHO ; And even for debugging purposes this log is not helping much... Nonetheless Symfony will automatically inject default logger which will consume resources, slowing parser down significantly.

It was added because users were getting issues with the parser without being able to share their private bookmark file on a public bug tracker. In non debug mode it only shows which line is parsed though, it isn't excessive IMO. Back to the point. If we make it optional, you could register your service with null and we could use a method like:

protected function log(...) {
    if ($this->logger instanceof LoggerInterface) {
        // log
    }
}

This shouldn't affect performances significantly.

Of course ; Note this change is absolutely necessary in order to have proper Symfony compatibility. Here's what I have in mind:

Yep that perfect. Out of curiosity, why is it absolutely necessary? I think that you can wire scalar variables to services in Symfony.

BTW, I have noticed that tags can own attributes like ADD_DATE and LAST_MODIFIED in some bookmark files. Having nested arrays representing both bookmark and tag would allow to map this data correctly.

:+1:

Having array properties renamed according to schema.org vocabulary will have the added benefit to integrate nicely with modern API standards

:+1:

Note that pub key should represent public/private instead of published.

[TangoMan75]

Hm I suppose it is. I don't really see a benefit of locking it though.

No benefit really... Just trying to apply best practices whenever possible.

It was added because users were getting issues with the parser without being able to share their private bookmark file on a public bug tracker.

Ah, OK... This is understandable.

If we make it optional, you could register your service with null and we could use a method like:

Well, I whish we could avoid it but since logging is a necessary feature, I like the idea to make it optional.

Yep that perfect. Out of curiosity, why is it absolutely necessary?

I misspoke ; I mean this is a feature I would absolutely need.

I think that you can wire scalar variables to services in Symfony.

Indeed, but I prefer have it to autowire without extra configuration steps out of the box.

Note that pub key should represent public/private instead of published.

Noted.

[ArthurHoaro]

@TangoMan75 Done finally! I'm going to add you as a maintainer of this project. Please follow these 2 simples rules:

• any change should be introduced through a pull request (leave some time for reviewer if it's a major change)
• any breaking change should be discussed and release under a new major version

[TangoMan75]

👋 Hi @ArthurHoaro,

Thank you for taking the time to merge my PR ! 🙏 I'm happy my contribution is useful.

And thank you for adding me to the project maintainers, I am glad to be a member of the team alongside @virtualtam, @kafene and @jvalleroy on this project.

I'll do my best and will get in touch with any questions or ideas I might have.

Also if there is anything I can do, please don't hesitate to reach out.

Cheers.