Skip to content

DocbookCS: remove SimparaSniff#5672

Open
vrana wants to merge 1 commit into
php:masterfrom
vrana:docbook-cs
Open

DocbookCS: remove SimparaSniff#5672
vrana wants to merge 1 commit into
php:masterfrom
vrana:docbook-cs

Conversation

@vrana

@vrana vrana commented Jul 11, 2026

Copy link
Copy Markdown
Contributor

I don't see any purpose for this rule. This argument makes no sense to me:

You can't have block elements in a simpara tags. Which helps prevent people writing dumb stuff inside para tags just because they think they might need it.

If people want to write dumb stuff inside para tags then they would just use them.

Moreover, simpara renders differently (the first uses simpara, second para):

image

Changing all para to simpara to satisfy this rule would completely break how the docs is rendered and what was the intent of the author.

@vrana vrana requested a review from jordikroon July 11, 2026 05:22
@jordikroon

Copy link
Copy Markdown
Member

This rule reflects the current style guide that has been in place for a while now.

image

So before we remove this, we have to re-evaluate the policy instead. Which in the broadest sense is a fair question.
The problem with <para> as a whole is that many abuse it. If it's used as <para>This function does X</para>, it's mostly fine. But it's been used in many other things.

The docs are flooded with <para>s as an outer of <itemizedlist>. Sometimes with inner text, sometimes without which makes the docs unnecessary complicated for new contributors. They don't know when to use <para> or <simpara>.

It's kind of the same in PHP where this is valid, but not preferred because it adds additional nesting which over a longer time degrades readability.

if ($foo) {
   if ($bar) {
   }
} else {
   if ($foobar) {
   }
}

The main problem is, the whole docs are mixed. As you pointed out on Discord, the list of violations are 91.794. So very much ideally we would go over it once, perhaps with [skip-revcheck] and fix it in bulk. That way nobody needs to be bothered with it anymore. And docbook-cs only picks up violations that are actually caused by the author.

As for the author, docbook-cs in diff mode doesn't give all the violations of the changed file, but only the lines the author have changed and the direct parent. So in short, if the author changes the inner text of <para>, and as a result it could be converted into a <simpara> it will mark it as a violation.

Moreover, simpara renders differently (the first uses simpara, second para):

IMHO this has nothing to do with simpara on its own. It's due to the fact that it renders incorrectly as a <span class="simpara">. Which in should be properly fixed in PHD.

This PR php/phd#246 should fix that behaviour. I will try to review and merge it in the upcoming days.

@vrana

vrana commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

The docs are flooded with <para>s as an outer of <itemizedlist>.

This rule doesn't address this issue. Exactly these problematic constructs will stay intact. The rule will "fix" only the places that need no fixing.

  1. There's XYZ amount of <para><itemizedlist>.
  2. Spend huge effort to replace all <para> with <simpara> where possible.
  3. There's still XYZ amount of <para><itemizedlist>.

If we see <para> as an outer of <itemizedlist> problematic then there should be a rule for that instead.

@jordikroon

Copy link
Copy Markdown
Member

Correct, but php/docbook-cs#17 does address that. Its impact both technical and on performance needs to be evaluated first. The CS fixer that Nick is working on could come handy when we add more rules along the way.

@vrana

vrana commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

Which brings us back to my original point. There are some problematic constructs. This sniff doesn't prevent them. Other sniffs will prevent them. So there's no purpose in this sniff.

@jordikroon

Copy link
Copy Markdown
Member

Why would this Sniff not have a purpose, if it follows the exact Docbook 5.2 spec. It suggests to use simpara where by spec it is allowed to be. It follows our Style Guide and is not problematic. For where it seems to be falsely problematic, is where PHD renders a span whereas it should be a p tag. That's a rendering error, not a sniff problem. Thus it should be fixed a PHD.

What this spec doesn't cover is exactly what I have mentioned, and there is a PR for that to cover it. It would not make sense to drop this Sniff just because. This Sniff by itself is complete.

The PR I did mention, suggests to omit the outer para where it's redundant.

@vrana

vrana commented Jul 11, 2026

Copy link
Copy Markdown
Contributor Author

I see no point neither in that style guide rule, nor in this sniff. I've asked many times what's the purpose of this rule in the style guide and this sniff. The purpose is not documented anywhere and nobody answered it to me.

The only answer I got is “People abuse <para>” which the style guide rule and this sniff does not prevent.

My point is that this sniff adds a lot of churn and doesn't solve any problem. Even running it on a diff triggers pre-existing errors. Copy-pasting text from elsewhere as well. If I run the CS on the whole manual, there's now thousands of errors which makes the full run unusable. Fixing the error in bulk would break translations. Also <para> is shorter and more natural to use. DocBook doesn't say that <simpara> is preferred.

What is all of this churn good for? Only to satisfy a styly guide rule which has no purpose.

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