ppkarwasz commented on code in PR #2072:
URL: https://github.com/apache/logging-log4j2/pull/2072#discussion_r1418010773


##########
log4j-core/src/main/java/org/apache/logging/log4j/core/net/UrlConnectionFactory.java:
##########
@@ -56,6 +56,17 @@ public class UrlConnectionFactory {
     private static final String NO_PROTOCOLS = "_none";
     public static final String ALLOWED_PROTOCOLS = 
"log4j2.Configuration.allowedProtocols";
 
+    @Deprecated(since = "3.0.0", forRemoval = true)
+    public static <T extends URLConnection> T createConnection(
+            final URL url,
+            final long lastModifiedMillis,
+            final SslConfiguration sslConfiguration,
+            final AuthorizationProvider authorizationProvider)
+            throws IOException {
+        return createConnection(
+                url, lastModifiedMillis, sslConfiguration, 
authorizationProvider, PropertiesUtil.getProperties());
+    }
+

Review Comment:
   Same as above.



##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java:
##########
@@ -121,6 +121,11 @@ protected boolean isActive() {
         return true;
     }
 
+    @Deprecated(since = "3.0.0", forRemoval = true)
+    public static ConfigurationFactory getInstance() {
+        return 
LoggerContext.getContext(false).getInstanceFactory().getInstance(KEY);
+    }
+

Review Comment:
   Not sure why this is required. I can not find any usage in this PR.



##########
log4j-jndi-test/src/main/java/org/apache/logging/log4j/jndi/test/junit/JndiRule.java:
##########
@@ -16,42 +16,113 @@
  */
 package org.apache.logging.log4j.jndi.test.junit;
 
+import static java.util.Objects.requireNonNull;
+import static org.junit.Assert.assertNotNull;
+
 import java.util.Collections;
+import java.util.Hashtable;
 import java.util.Map;
+import java.util.Set;
+import java.util.Spliterators;
+import java.util.stream.Collectors;
+import java.util.stream.StreamSupport;
+import javax.annotation.Nullable;
 import javax.naming.Context;
+import javax.naming.NameClassPair;
+import javax.naming.NamingException;
+import javax.naming.spi.InitialContextFactoryBuilder;
+import javax.naming.spi.NamingManager;
+import org.apache.logging.log4j.jndi.JndiManager;
 import org.junit.rules.TestRule;
 import org.junit.runner.Description;
 import org.junit.runners.model.Statement;
-import org.springframework.mock.jndi.SimpleNamingContextBuilder;
+import org.osjava.sj.jndi.MemoryContext;
 
 /**
  * JUnit rule to create a mock {@link Context} and bind an object to a name.
  *
  * @since 2.8
  */
+@SuppressWarnings("BanJNDI")
 public class JndiRule implements TestRule {
 
-    private final Map<String, Object> initialBindings;
+    static {
+        final InitialContextFactoryBuilder factoryBuilder =
+                factoryBuilderEnv -> factoryEnv -> new MemoryContext(new 
Hashtable<>()) {};

Review Comment:
   Why not storing this `MemoryContext` somewhere?
   
   The `NamingManager.setInitialContextFactoryBuilder` method is irreversible 
(can be called only once), so the context used by 
`JndiManager.getDefaultManager()` will always be the one defined in here.



##########
log4j-jndi/src/main/java/org/apache/logging/log4j/jndi/JndiManager.java:
##########
@@ -194,6 +195,14 @@ protected boolean releaseSub(final long timeout, final 
TimeUnit timeUnit) {
         return JndiCloser.closeSilently(this.context);
     }
 
+    /**
+     * @return the active context
+     */
+    @Nullable
+    public Context getContext() {
+        return context;
+    }

Review Comment:
   I have mixed feeling about exposing `Context` (which is usually 
`InitialContext`, the source of all evil) from the public API.



-- 
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: [email protected]

For queries about this service, please contact Infrastructure at:
[email protected]

Reply via email to