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

Implement rules federation, fix markdown federation #694

Merged
merged 5 commits into from
Apr 28, 2024

Conversation

BentiGorlich
Copy link
Member

  • the summary of magazines includes the markdown source now
  • the summary of magazines includes the rules now
  • when updating a magazine the markdown source is used if available and if the last header includes "rules" it will be put in the rules field
  • the summary actually works now. Before it would output the markdown as the summary and we would assume that the summary field is an html field, so we would try to convert html -> markdown, except that the html we tried to convert already was markdown, so it just didn't work properly

Closes #415

- the summary of magazines includes the markdown source now
- the summary of magazines includes the rules now
- when updating a magazine the markdown source is used if available and if the last header includes "rules" it will be put in the rules field
- the summary actually works now. Before it would output the markdown as the summary and we would assume that the summary field is an html field, so we would try to convert html -> markdown, except that the html we tried to convert already was markdown, so it just didn't work properly
@BentiGorlich BentiGorlich added bug Something isn't working enhancement New feature or request activitypub ActivityPub related issues backend Backend related issues and pull requests labels Apr 8, 2024
@BentiGorlich BentiGorlich added this to the v1.6.0 milestone Apr 8, 2024
@BentiGorlich BentiGorlich self-assigned this Apr 8, 2024
@BentiGorlich
Copy link
Member Author

I observed the problem especially while looking at https://gehirneimer.de/m/FloatingIsFun@fedia.io vs https://fedia.io/m/FloatingIsFun
Which just looks hilariously bad...

Comment on lines 809 to 836
public function extractRules(string &$description): ?string
{
$lines = explode("\n", $description);
$headers = [];
$i = 0;
foreach ($lines as $line) {
if (preg_match("/^#+ ([^\n\r]*)/", $line, $matches)) {
if (2 === \sizeof($matches)) {
$headers[] = ['line' => $i, 'header' => $matches[1]];
}
}
++$i;
}

$rules = null;

if (!empty($headers)) {
$lastHeader = $headers[\sizeof($headers) - 1];
if (str_contains(strtolower($lastHeader['header']), 'rules')) {
$descLines = \array_slice($lines, 0, $lastHeader['line']);
$ruleLines = \array_slice($lines, $lastHeader['line'] + 1, \sizeof($lines) - $lastHeader['line']);
$description = join("\n", $descLines);
$rules = join("\n", $ruleLines);
}
}

return $rules;
}
Copy link
Member

Choose a reason for hiding this comment

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

I just don't think this is a good idea vs parsing it as a regular description like we've been doing, it seems way too prone to failure. For instance a random remote magazine might have a quote in its description like ## “Know the rules well, so you can break them effectively.” - Dalai Lama XIV we suddenly parse that as the rules. Also, it doesn't work for non-english, so is flaky

Copy link
Member Author

Choose a reason for hiding this comment

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

Before thinking to hard about this what do you think about my ideas for the rules field? Because if you think it is too complicated and not really worth it then we will just deprecate the field (if everyone agrees)

Copy link
Member

@e-five256 e-five256 Apr 9, 2024

Choose a reason for hiding this comment

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

I think it's fine to keep it if you see value

My opinions on keeping it would be:

  • Add it as "Rules" as you have now on outgoing
  • Remove this extract rules from remote, just keep it all in description for now, even from other mbin instances
  • Make an FEP for something like adding a rules field ordered list to group actors, if that's what you think works better

@TheVillageGuy
Copy link
Contributor

I'm sorry for being a bit late to the party, but isn't this related to a feature request I made a while ago asking for local and external description/rules. I'm having a bit of a hard time grasping what exactly you're handling here so can't say whether I agree or not

@BentiGorlich
Copy link
Member Author

If I remove the dev / AP specific jargon, I did the following:

  • add the rules to the description so they get federated to other instances (e-five correctly criticized that I add a non translated "Rules" header)
  • when getting the info of a remote magazine, we "parse" the description of it to extract rules into our "Rules" field. That is error prone and only works for magazines with an English description (e-five explained why it is error prone)

The "parsing" will most likely be removed

@nobodyatroot
Copy link
Member

i vote for whatever gives us the most/best compatibility across the fediverse :-)

@BentiGorlich BentiGorlich merged commit 8b24c69 into main Apr 28, 2024
7 checks passed
@BentiGorlich BentiGorlich deleted the new/magazine-desc-markdown-and-rules branch April 28, 2024 12:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
activitypub ActivityPub related issues backend Backend related issues and pull requests bug Something isn't working enhancement New feature or request
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Bug] Rules are not federated
4 participants