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

Fix crashes caused by system installed extensions. #129

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ohrn
Copy link
Contributor

@ohrn ohrn commented Mar 12, 2018

This is a quick fix to avoid crashes in a couple of places. System extensions do not have a Critic user account set as author.

An improved fix for the future would be to use the author information in the extension manifest.

System extensions do not have an author set.
@jensl
Copy link
Owner

jensl commented Mar 19, 2018

"Imported" as https://critic-review.org/r/415 for testing. I'll merge this to the stable/1 branch first. That's the branch you will want to follow anyway.

On your suggestion for an improved fix: in some cases, maybe. But really there are two roles involved: the "author", which is defined in the MANIFEST file, and of which there can be many; and the "publisher", who is the Critic user in whose home directory the extension lives. System extensions have no publisher, but they still have one or more authors. But the author would not necessarily map to a user account on the Critic system.

To make this more confusing, the "publisher" role is called "author" in many places in the code. It's has traditionally been the same human being, but its really two distinct roles.

So: in some cases, the actual author was intended, in which case the information from the MANIFEST file should have been used. (This is never stored in the database, though, so somewhat less available to some parts of the code.) In other cases, the publisher was intended, and those places should have a null check, to support system extensions.

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.

2 participants