It's also possible we may only want to do this in Vector, or something, just to distinguish more between the two skins - they have the exact same behaviour regarding this now, but that doesn't necessarily mean that's desired either.

Change 518092 had a related patch set uploaded (by Isarra; owner: Isarra):
[mediawiki/skins/MonoBook@master] Move DataAfterContent outside of main content block

https://gerrit.wikimedia.org/r/518092

Change 518095 had a related patch set uploaded (by Isarra; owner: Isarra):
[mediawiki/skins/Vector@master] Move DataAfterContent outside of main content block

https://gerrit.wikimedia.org/r/518095

FlaggedRevs in vector before and after:

Change 518095 merged by jenkins-bot:
[mediawiki/skins/Vector@master] Move DataAfterContent outside of main content block

https://gerrit.wikimedia.org/r/518095

Change 518092 merged by jenkins-bot:
[mediawiki/skins/MonoBook@master] Move DataAfterContent outside of main content block

https://gerrit.wikimedia.org/r/518092

Oh my. :D

Huh, neat.

It feels weird to see the FlaggedRevs box (or anything, really) outside of the blue content box, after getting used to how the footer looked like for years. But I agree it's more logical!

This changed causes layout problems with SMW: "Move DataAfterContent outside of main content block" breaks Factbox display.

Is there any way to achieve what you want without breaking other extensions? Is there a way we could be consulted on changes like this before being surprised by them in the code?

In T226199#5331432, @MarkAHershberger wrote:

This changed causes layout problems with SMW: "Move DataAfterContent outside of main content block" breaks Factbox display.

Is there any way to achieve what you want without breaking other extensions? Is there a way we could be consulted on changes like this before being surprised by them in the code?

Respectfully, it seems like Factbox was using the wrong hook for this to begin with. Even before the change, the hook placed things underneath the Categories box and whatever other post-content metadata there was. For something which seems to be actual content, that seems like an odd place to put it. I would suggest either looking into a different hook, or going the route that Extension:Cite uses and make a <factbox /> tag extension that the user can put wherever they want on the page (much like Cite's <references /> tag). This has the added advantage of letting the user choose where on the article they want the factbox, such as being able to float it in a sidebar or having it below an introductory paragraph.

For the other question, as much as it pains me to say it as a 3rd-party extension developer myself, it is quite simply impossible to test every change in core for compatibility with every extension, and block the changes if such compatibility issues arise. For notifications on when something impacts you, you can sign up for notifications whenever relevant patches are added to gerrit. It's not perfect because there may be some noise due to patches which don't impact your extension, but it will give you instant notification whenever impactful patches are uploaded, and gives you a chance to test compatibility and comment on the patch before it gets merged in. If you find or suspect a major compatibility issue, adding a Code Review -1 is a good way to help guide the patch towards something that would work better with your extension. Of course, it is entirely possible that the extension itself is at fault (due to using deprecated methodology, or simply using a hook in an exotic way), in which case the changes are likely better done on the extension rather than in core; at least this way you will be aware of the need for such changes rather than being blindsided upon a version release.

As an example, Timeless and Bluesky are two other skins that would already have been placing the Factbox thus, having effectively taken the hook at its word that it is data after the content; this just makes Vector and MonoBook more consistent with this naming and expected usage (as anything that is content can simply be added to the content itself). Other skins such as Modern and GreyStuff are unchanged because they don't really have clear separation in the first place.

As this makes the skins more consistent, updating Factbox to account for this should likewise make it more compatible across all skins, which was sort of the point of the change in the first place - by consolidating skin behaviour, we minimise the amount of extra effort required by extension developers to make their extensions compatible across the board.

It's not reasonable to expect any developer to monitor all the (proposed) changes happening in the code they use. When I used to do that, I would spend over 50% of my time on it. MediaWiki has evolved beyond what you suggest and it has more mature processes to deal with change.

In this case it looks like you knew you were doing a significant change. Maybe you did not realize it was a breaking change for some, but you ought to have checked that out before merging. With code search it is often trivial to look up which code (if any) is affected and proactively reach out the maintainers to inform or to get feedback or even provide fixes. Because Vector is the default skin for MediaWiki, I find it reasonable to hold it to the same standards as core: follow the deprecation policy, do due diligence to avoid breaking other code and provide clear migration paths. For less used skins such as Timeless, I wouldn't be so strict.

TL;DR: It is not okay to break other code like this and forcing other developers to deal with the breakage with no advance notification and with no clear migration path.

https://xkcd.com/1172/

I'm also a little confused what actually constitutes 'breakage' here to begin with, given that the factbox still appears to be displaying fine, just slightly further down. How deep do we need to go with this to catch potential issues, a full UX design review of every extension using the hook?

(We're talking about an extension using a hook that exists to insert things after the content to insert things into the content. A quick test shows the extension content to be displaying reasonably for the described hook purpose; if we're expected to catch that it's actually trying to do something else entirely, that requires a much more thorough review of its function and interaction.)

I listed some suggestions above how to check for potentially affected code. However, the change has been merged already and a regression has been reported. Now would be time to evaluate whether the change can be reverted or quickly fixed so that:

1. the existing functionality is restored, or
2. the change can be done in a way that doesn't cause the breakage (e.g. new hook for new behavior), or
3. the affected code has a clear migration path and a migration period (e.g. new hook or switch for old behavior), or
4. the affected code hasn't a migration path, but there is a migration period with justification why the functionality cannot no longer be supported in the future, or
5. the affected code hasn't a migration path, and there is no migration period with a very strong justification why it must be so

I’m going to be a bit more blunt here, since that seems that is required.

There is no regression. The code in question was using the wrong hook to begin with, relying on an implementation detail on certain skins that wasn’t documented or even across all other skins. In the sense that the hook displays the stuff after the article content and metadata (aka the documented behavior), that still holds true.

So, the correct answer is: 6. Fix the extension to use a more appropriate hook or other mechanism to inject content instead, because it was always using the wrong thing.

wrong hook to begin with

The documentation states: "Allows extensions to add text after the page content and article metadata." and other code relies on the previous behavior. It doesn't matter whether it was right or wrong.

Fix the extension to use a more appropriate hook

What is the more appropriate hook with instructions how to migrate to it, in a way that backwards compatibility can be preserved?

In T226199#5337077, @Nikerabbit wrote:

The documentation states: "Allows extensions to add text after the page content and article metadata." and other code relies on the previous behavior. It doesn't matter whether it was right or wrong.

That is still true; data injected by the hook appears after the page content and article metadata. The documented behavior is unchanged. The documentation does not state that the hook injects the data as part of the main content box. Indeed, that only held true for monobook and vector. Other skins such as timeless already put it afterwards in its own container, based on what Isarra said.

I have already answered your other question above in my first comment. This extension seems like it is trying to add additional content to the page, so using a hook that puts it below metadata seems odd. Other extensions which use this hook, such as FlaggedRevs, RelatedArticle, and pretty much everything else use it to inject non-content afterwards that is still related to the page. Moving it into its own content box doesn't "break" any of these because none of them were trying to pretend that they were content. The solution for this extension would be to either use a more appropriate hook to inject it as part of the content, or make it a tag extension instead and let the user determine where in the content they wish to place it.

I'm going to explicitly second Skizzerz' original recommendation: this extension should be using a parser hook, as Cite does.

In T226199#5337512, @Isarra wrote:

this extension should be using a parser hook, as Cite does.

But it isn't and things were working and this breaks the developer's expected outcome.

The proper way to introduce this change is to add a new hook and deprecate the old one. That allows you to introduce needed changes and notify developers of a needed change without disrupting the current behavior of functions that currently use it.

In T226199#5337577, @MarkAHershberger wrote:

The proper way to introduce this change is to add a new hook and deprecate the old one. That allows you to introduce needed changes and notify developers of a needed change without disrupting the current behavior of functions that currently use it.

It absolutely is not. That would break every other extension using the hook, and require adding new functionality not just to core, but to every skin that already exists, for no reason at all than that a single extension was misusing an already clearly-defined hook that is already largely consistently implemented across skins. That would in fact constitute a real breakage.

In T226199#5337603, @Isarra wrote:

In T226199#5337577, @MarkAHershberger wrote:

The proper way to introduce this change is to add a new hook and deprecate the old one. That allows you to introduce needed changes and notify developers of a needed change without disrupting the current behavior of functions that currently use it.

It absolutely is not. That would break every other extension using the hook, and require adding new functionality not just to core, but to every skin that already exists, for no reason at all than that a single extension was misusing an already clearly-defined hook that is already largely consistently implemented across skins. That would in fact constitute a real breakage.

I respectfully disagree. Expected, accepted, long standing behavior was changed without warning, breaking (i.e. causing unanticipated, undesirable changes to the display of) at least one existing extension as shown at https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/4146. There indeed may be other extensions using the hook, and it cannot be assumed whether they would like the new behavior. A change of this magnitude to existing behavior should have been widely discussed. And, an equitable solution would be to retain the existing behavior, potentially under a new name if an appropriate deprecation period is observed, an add the new behavior as an alternative that extensions could opt into.

OK, taking a step back.

The intention of this change was to standardize how all skins display content using the SkinAfterContent hook (that is, moving it explicitly after the content area, as the name implies). I think we all agree that long-term the skin standardization that @Isarra, @Jdlrobson, and others are working on is a good thing, yes?

If so, I think we should be looking for ways to move forward rather than an attitude of "don't change stuff ever, because it might break things".

I've looked at the SMW screenshot, and aside from it being in a different place, it's unclear to me exactly what's broken. What am I missing?

Change 523772 had a related patch set uploaded (by markahershberger; owner: markahershberger):
[mediawiki/core@master] Add hook to provide a path migrate away from DataAfterContent

https://gerrit.wikimedia.org/r/523772

In T226199#5337828, @Legoktm wrote:

If so, I think we should be looking for ways to move forward rather than an attitude of "don't change stuff ever, because it might break things".

This is what deprecation is for: making changes in a way so that new functionality can be implemented in a non-disruptive way.

See my WIP patch for a suggested way forward.

For the Factbox extension: is this part of the page content? Or is it not page content, but rather information about the page, related, etc?

It's one or the other:

• If it's not page content, but merely related information, then it is using the correct hook. What is the issue with its new placement in Vector, exactly?
• If it is page content, then it should be added to the page content directly (we have already explained how to do this). The skin should not be affecting anything, nor need to know about it to begin with (beyond maybe some skinstyles, but that's a totally separate matter). It should not be appearing in the footer, for instance, in Minerva, or among the page tools and metadata page footer in Owl/BlueSky. It should be with the content, in the content block.

In T226199#5337955, @MarkAHershberger wrote:

In T226199#5337828, @Legoktm wrote:

If so, I think we should be looking for ways to move forward rather than an attitude of "don't change stuff ever, because it might break things".

This is what deprecation is for: making changes in a way so that new functionality can be implemented in a non-disruptive way.

Deprecation isn't a magic wand, it has to balance out obligations on the developer trying to change things vs people trying to use things as a stable platform. I'd like to think we've achieved that balance in MediaWiki core, but no such policy exists for the Vector skin, so asking for one to be retroactively applied seems a bit unfair.

In any case, I think we're still stuck because it's not clear exactly why this change was problematic, like I asked earlier:

In T226199#5337828, @Legoktm wrote:

I've looked at the SMW screenshot, and aside from it being in a different place, it's unclear to me exactly what's broken. What am I missing?

In T226199#5337828, @Legoktm wrote:

OK, taking a step back.

The intention of this change was to standardize how all skins display content using the SkinAfterContent hook (that is, moving it explicitly after the content area, as the name implies). I think we all agree that long-term the skin standardization that @Isarra, @Jdlrobson, and others are working on is a good thing, yes?

Yes.

If so, I think we should be looking for ways to move forward rather than an attitude of "don't change stuff ever, because it might break things".

There is a middle ground between leaving everything the same and making a change: announcing a change is coming, having a deprecation period for those relying upon the existing functionality to adapt to the change, and, if necessary, introducing an alternative for those that rely upon the previous behavior.

I've looked at the SMW screenshot, and aside from it being in a different place, it's unclear to me exactly what's broken. What am I missing?

At https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/4146 you can see the before and after screenshots. In the Before, the factbox is after the rendered wikitext content of the page and categories but still inside the box around the content area. In the After, you can see in the part circled in red that the factbox is in a separate box. This is a significant semantic change from a UI perspective. Originally, the factbox was visually linked to the content. After, it is outside the area with the navigational elements.

Absolutely, one could argue that the original intent of the hook was the new behavior, but the behavior was otherwise for a significant period of time, and others relied upon that behavior. It is not unreasonable to expect warning for such a change as well as a way to retain the old behavior.

In T226199#5337828, @Legoktm wrote:

The intention of this change was to standardize how all skins display content using the SkinAfterContent hook (that is, moving it explicitly after the content area, as the name implies). I think we all agree that long-term the skin standardization that @Isarra, @Jdlrobson, and others are working on is a good thing, yes?

I agree. The intention to standardize is good.

That said "after content" means "after content" not "outside the content div". It could even be understood as "inside the content div, but placed after the content" which was certainly the practice of the vector skin.

The documentation only says "Allows extensions to add text after the page content and article metadata." The practice of the vector skin (the skin used by a plurality of wikis afaict) up until this point is to place this after the content but inside the content div.

This is why my patch provides SkinOutsideContent--although, to make it really clear, I think I'll change it to SkinOutsideContentRegion. That way, we can have a deprecation period for the more ambiguous "SkinAfterContent" and have a period for skin creators and developers to adapt to the less ambiguous SkinOutsideContentRegion.

In T226199#5339823, @Legoktm wrote:

Deprecation isn't a magic wand, it has to balance out obligations on the developer trying to change things vs people trying to use things as a stable platform.

And that is what the new hook does.

Copying this comment of mine from code review because I think this is a clear summary of my argument:

People have used SkinAfterContent to put things *after* the content but inside the area labeled "content". This [patch] says "ok, you may have done that in the past, but that isn't what we meant. Let us clarify it and give you a grace period to adapt to our clarification."

Would you like to work with us on improving the affected extension, or are you guys just here to complain? Let's start over. Please, file a task for the specific issue here, describe it fully and what the assumptions and expectations are, across all skins, and we can work on discussing the best way to address this. Let's try to move forward productively.

In T226199#5340991, @CCicalese_WMF wrote:

<snip>
This is a significant semantic change from a UI perspective. Originally, the factbox was visually linked to the content. After, it is outside the area with the navigational elements.

Yes, it's a change. The question I was asking is what specifically is wrong with the change. If it's supposed to be in the content area (what I interpret "visually linked to the content" to mean, please correct me if I'm wrong), then the previous behavior wasn't right at all - it was inconsistent across skins (and there are already better skin-agnostic ways to do so).

As the person who merged the change, I'm sorry we missed this and broke this. Knowing how extensions interact with skins, the assumptions they make and the fact they are so open ended is a continued source of frustration and anxiety to me [1]. Clearly mistakes were made and we need to work out a better deprecation process here. On the other hand we are talking about them so that's a good thing and the priority should now be fixing the problem in Semantic MediaWiki while not regressing the features this attempted to help.

.To be honest, if I'd known about this situation my process would have been

As a skin maintainer Id like to minimise hooks and reliance on HTML template values - they are very a fragile API and I'd like to see less of them (deprecated or not) and replaced with a proper skin API e.g. Skin::addMenuItem. As far as I'm concerned html-dataAfterContent/dataAfterContent should be considered a skin internal and not a public API. The only reason it is used is because of an absent of an API. My team is currently thinking about refreshing the Vector skin and one of the things we are thinking about is the architecture of skins as a whole to make this less brittle

I believe we now have a situation where if we do retroactively deprecate we regress the things this change fixed in subsequent patches (RelatedArticles and PageTriage).

What is the downside of fixing SemanticWiki usage retroactively using one of the more skin-agnostic ways like @Legoktm proposes? Is help needed here?

[1] A codesearch across Wikimedia didn't show any results for dataAfterContent so it was not clear that https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/518095/1/includes/templates/index.mustache would impact SemanticMediaWiki.

In T226199#5342865, @Jdlrobson wrote:

What is the downside of fixing SemanticWiki usage retroactively using one of the more skin-agnostic ways like @Legoktm proposes? Is help needed here?

Patches against SMW are welcome to help fix the problem in a backwards compatible way. Any fix should also work with MediaWiki back to 1.31.

I think the main issue was the surprise factor. The change was only discovered because the developer happened to test the skin the weekend after the change was made.

If you're right that "html-dataAfterContent/dataAfterContent should be considered a skin internal and not a public API" then they should be marked as such with proper warning given.

As it stands, people were using a publicly documented, not-deprecated APIs with the expectation of stability. If people should not depend on the semantic structure remaining the same, then that should be documented.

I suggest that the proper way to do this (without any deprecation, if you would like to do it that way) is to say "in version 1.3x we are changing the skinning system to make it more rational and less brittle."

Making the change is good. Making it without communicating your direction to those who depend on you is bad.

In T226199#5342865, @Jdlrobson wrote:

[1] A codesearch across Wikimedia didn't show any results for dataAfterContent so it was not clear that https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/Vector/+/518095/1/includes/templates/index.mustache would impact SemanticMediaWiki.

That is because you were looking for the template item (dataAfterHook), not the code that populates it in [[ https://gerrit.wikimedia.org/r/#/c/mediawiki/core/+/523772/3/includes/skins/Skin.php | afterContentHook ]].

I made the same mistake initially and couldn't find the change. Only after a bit of reading did I realize that the name for the template item is different than the hook name.

@Jdlrobson, thank you for your comment. I agree that it is best at this point to focus on the future: a backward-compatible fix for Semantic MediaWiki (either in the extension, skins, or both) and trying to prevent similar issues in the future. The issue in Semantic MediaWiki is documented at https://github.com/SemanticMediaWiki/SemanticMediaWiki/issues/4146, if anybody would like to comment or contribute there.

I think OutputPage::addHTML is probably the best way to insert this content after the parser output and used inside the BeforePageDisplay hook. @Legoktm would you agree? If so I can provide a patch providing someone is at hand to test.

I suggest communicating with the Semantic MediaWiki maintainer, mwjames, on the issue on github before taking any action. He generally only works on the extension on weekends. It could be that he already has a plan in mind to fix the issue, so you may not want to jump into coding something up quite yet. And, he seemed quite sure that BeforePageDisplay would not work for his use case.

Given how heated this has gotten already, I'm not sure I would necessarily recommend continuing the discussion in a space not covered by the Wikimedia code of conduct.

If anyone here has wants to contribute code to the SMW project, but hesitates because of SMW is not governed by the WMF's CoC, I will work with them to privately contribute their code with or without attribution, whichever is preferred. I voluntarily place my collaboration in this mode under the WMF's CoC and any arbitration there.

I'm doing this because I want to avoid any chilling effect that the absence of the WMF's CoC may have on collaboration with the SMW project.

Change 523772 abandoned by markahershberger:
Add hook to provide a path migrate away from SkinAfterContent

https://gerrit.wikimedia.org/r/523772