ppkarwasz commented on PR #3839:
URL: https://github.com/apache/logging-log4j2/pull/3839#issuecomment-3206986816
> 1. It duplicates quite some logic (e.g., in `JsonConfigurationFactory`,
extensions and suffixes are semantically identical).
Totally agree. Let’s put a **default** implementation in
`ConfigurationFactory` so most factories don’t have to repeat themselves.
Something like:
```java
@Override
public List<String> getSupportedExtensions() {
final String[] types = getSupportedTypes(); // protected, legacy API
// Preserve legacy semantics: only advertise extensions when active and
types are present.
return isActive() && types != null && types.length > 0
? Arrays.asList(types)
: Collections.emptyList();
}
```
> 2. Supported configuration file extensions *could* be obtained from
`ConfigurationFactory.getFactories()` by filtering on `isActive()` and
collecting `getSupportedTypes()` *if* only `getFactories()` would have existed.
Put another way, why don't we add a static `getFactories()` to `CF` and be done
with it?
I prefer the dedicated `getSupportedExtensions()` API for a few reasons:
* `isActive()` and `getSupportedTypes()` are **protected**, not public, so
they’re not available to consumers; exposing them (or a raw `getFactories()`)
would leak internal details.
* Implementations are **inconsistent** today: some factories always report
active but conditionally restrict types (e.g., Log4j 1 XML returns `xml` only
when a property is set), while others return a constant type set but flip
`isActive()` based on optional deps. A single public method lets us
**normalize** that behavior and present a stable contract.
* In Log4j 3, `isActive()` isn’t needed anymore: optional Maven **modules**
provide support for additional formats, not per-factory optional dependencies,
so I would propose to remove `isActive()` from Log4j 3.
* Conceptually, `ConfigurationFactory` should look like a **singleton
service**; how it multiplexes concrete factories is an implementation detail.
Exposing `getFactories()` would couple users to that detail, whereas
`getSupportedExtensions()` gives them exactly what they need without exposing
internals.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscr...@logging.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
