Make old core document events official public API

Development
#1

Hi everyone,

@MichaelHamann recently brought my attention on the fact that I’m using in Change Request an internal event from oldcore: UserUpdatingDocumentEvent. And more generally I think we often use in extensions or custom code (or even advise to use here in the forum), the events that are available in xwiki-platform/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/internal/event at master · xwiki/xwiki-platform · GitHub.

For example I’m sure I already suggested people to use the XObjectUpdatedEvent without realizing it was internal. IMO we cannot consider those events are only internal: I’m pretty sure some of them are now used in lots of code outside of XS and modifying / removing that code would be quite a breaking change.

So my proposal here is to make official that all those events are public API. For doing that, we sadly don’t have much choice: as I said we cannot remove that code, which means we can’t refactor the package name as it would break extensions. Then the only choice seems to be to deprecate those events and re-create them with a new package.

wdyt?

#2

These events are internal so it’s ok to break them IMO. It’s the contract! If people start to use internal package then it’s at their own risks. It’s documented. If we don’t do this then let’s drop the idea of internal then…

Developers of extensions using them will need to release new versions. To help, we can also easily check where they’re used in xwiki-contrib using GH’s search, and fix them if need be.

Now, if we really don’t want to do this (and I think we should do it, it’s just our rule) then they’d need to be deprecated as you said, but also moved to legacy.

+0 to make these events public. idk if they should all be public or only some (ideally their APIs should be reviewed to verify they’re good and it’s still time to change them, it’s possible we took shortcuts because they were internal and not meant to be exposed for example).

Thanks

#3

I forgot one aspect: we need to review them in the context of other events that are already public. There might be duplicates.

Last, it would be good to list them all somewhere on xwiki.org in some reference documentation (maybe in https://extensions.xwiki.org/xwiki/bin/view/Extension/Observation%20Module%20Local or in the extension itself at https://extensions.xwiki.org/xwiki/bin/view/Extension/Observation%20Module or even better move them from oldcore to another module, document them in that extension doc on exo and link from https://extensions.xwiki.org/xwiki/bin/view/Extension/Observation%20Module%20Local ).

It’s not that great to make public code from oldcore, would be better to move it in another module first. We started putting “good” events in xwiki-platform-bridge in the past (but the bridge part is no longer great nowadays…).

#4

For the core events (ofc any, extension can contribute more, see Where can I find a list of Event Listeners? - #2 by mflorea )

#5

I’m +1 to have public versions of those very usefull events. Now I’m not sure all of them needs to go through deprecation: for example, while it would definitely be a pain for you, UserUpdatingDocumentEvent is not super old (OK, we might not have the same scale here :slight_smile: ), and I really doubt it was used in any other extension yet.

The problem is that the only way to fix that is to upgrade your extension to a version of XWiki where the new event exist (or reimplement all the code which produce those events on your extension side, which is complex for some events and simply not possible in the case of UserUpdatingDocumentEvent).

Well it’s not the only way, actually, you can use reflection, but that’s painful.

#6

Indeed, good point.

After my last comment I’m wondering if we shouldn’t move all core events to a single module (ie the ones from bridge and oldcore). platform-model was supposed to be for the “new” model :wink: and thus stay clean of old model apis. And bridge was an effort to introduce an intermediary api between the old core api and before the new model was ready.

#7

+1 to make these events public. It’s a pain not to have them properly available in extensions.

#8

Also it’s not clear to me why we still need DocumentUpdatingEvent when there’s now UserUpdatingDocumentEvent. AFAICS we can (and should probably) always pass a user responsible for the modification. Thus, shouldn’t DocumentUpdatingEvent be deprecated?

I’m asking here because if we make UserUpdatingDocumentEvent public then the question arises IMO.

#9

No. The point of UserUpdatingDocumentEvent is not to know which user is the source of some modification (you already have this information in theory), it’s to know that a change has been made directly by a user (and not some indirect modification made through some application already in charge of checking what a user is allowed to do or not) so that you could add special protection to it (for example make sure some user with edit right does mess with the rights).

#10

ok thanks, I suggest to improve the javadoc from the class since that’s where I got this idea that it was to add the information about the user responsible for the modifications.

Here’s the current javadoc:

  • Same as {@link DocumentUpdatingEvent} but with an information about which user is responsible for the modifications.
#11

Yes, the documentation should be improved in the public version.

#12

Actually we do have lots of events already in xwiki-platform-bridge, see: xwiki-platform/xwiki-platform-core/xwiki-platform-bridge/src/main/java/org/xwiki/bridge/event at master · xwiki/xwiki-platform · GitHub so it would make sense to put them all there probably

#13

Yes, we already have public events, hence my first remark about making sure that exposing the new events doesn’t duplicate stuff that we have in bridge already.

Now, I’m not sure i like the idea of adding more stuff in the bridge module, for 2 reasons:

  1. The bridge module was an attempt at an API independent of the old model api (it doesn’t have a dep on oldcore) and I think there are oldcore apis in the oldcore events (that’s why they were added there btw afair)
  2. I don’t think that the bridge module is a future-proof module, with the “bridge” term in the package name, but that’s to be decided (see below).

Hence my comment above:

I think we need to rediscuss how we want the following modules to evolve in the future:

  • xwiki-platform-model
  • xwiki-platform-bridge

Another option would be to add a new xwiki-platform-events module or maybe even better a xwiki-platform-observation-events module for core model events. But while it’s tempting to have something clean, it may fragment even more where we put model-related stuff…

Maybe the best is just to continue or current strategy and put oldcore-dependent stuff in oldcore, but move the event classes to some org.xwiki.event package (which would also work if one day we move them to a xwiki-platform-events module). It’s not the best place since the ideal package name would be org.xwiki.model.event but that’s reserved for the “new” model.

WDYT?

PS: We didn’t mention it above but all the events in xwiki-platform/xwiki-platform-core/xwiki-platform-oldcore/src/main/java/com/xpn/xwiki/internal/event at master · xwiki/xwiki-platform · GitHub are in the old and deprecated com.xpn.xwiki package and we need to change that.

#14

Another argument to not break at least the XObject events is that it’s actually exposed in a public API… :sweat_smile:

#15

I really don’t think adding another modules for those low level oldcore API related events make sense. Those events should go in either oldcore (my preference) or the bridge.