[ 
https://issues.apache.org/jira/browse/WICKET-6600?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Gert-Jan Schouten updated WICKET-6600:
--------------------------------------
    Description: 
As a result of these three commits:
 * 
[https://github.com/apache/wicket/commit/dec1293da853357f483eadec4616c9758ea025f9#diff-8964e43671328bf6f9b1d75703103db8]
 * 
[https://github.com/apache/wicket/commit/3be55cf5a24982cfca400225aabb6c4296859a7d#diff-8964e43671328bf6f9b1d75703103db8]
 * 
[https://github.com/apache/wicket/commit/830fb15c333ad6099f466b2344a84b513690db86#diff-8964e43671328bf6f9b1d75703103db8]

Wicket now logs a stacktrace on error level if you add something to the 
AjaxRequestTarget that has been removed from the page. I don't think this is a 
good idea, because you're not always in control of what happens in an Ajax 
call. Imagine, for example, the following scenario:
{code:java}
public abstract class MyPanel extends Panel {

    public MyPanel(String id) {
        super(id);

        this.setOutputMarkupId(true);

        this.add(new AjaxLink("myLink") {
            public void onClick(final AjaxRequestTarget target) {
                this.executeImportantWork(target);

                //Display something nice on this Panel
                //...

                target.add(MyPanel.this); //In order to show the nice things
            }
        });
    }

    protected abstract void executeImportantWork(AjaxRequestTarget target);
}

{code}
Now, what if the user of MyPanel actually adds it to the Page and then, in 
executeImportantWork, removes it and replaces it with smth else? Then, you get 
an error in the log, everytime someone clicks myLink.

In this case, it could be fixed by adding MyPanel.this to the target *before* 
calling executeImportantWork, in which case you also get something in the log, 
but without stacktrace and on WARN level, namely this: "Component not rendered 
because it was already removed from page", but this is far less invasive than 
the scary stacktrace on ERROR level.

In other cases, however, it may be more difficult to fix, for example when 
you're not in control of the source code of MyPanel. Sure, one could argue that 
you shouldn't alter the page hierarchy in the executeImportantWork method, but 
usually, there's always a certain edge case where 99% of the times, no page 
hierarchy change occurs, but that 1% of the times, it is necessary.

Wicket is a component based framework, so it should be possible to specify 
components with abstract behaviour and not restrict the user of those 
components in what they can do in the implementation of that abstract 
behaviour, especially since, in this case, it does not lead to any actual 
problems and can safely be ignored with a warning.

I suggest changing the level of the message to WARN and removing the stacktrace.

  was:
As a result of these three commits:
 * 
https://github.com/apache/wicket/commit/dec1293da853357f483eadec4616c9758ea025f9#diff-8964e43671328bf6f9b1d75703103db8
 * 
https://github.com/apache/wicket/commit/3be55cf5a24982cfca400225aabb6c4296859a7d#diff-8964e43671328bf6f9b1d75703103db8
 * 
https://github.com/apache/wicket/commit/830fb15c333ad6099f466b2344a84b513690db86#diff-8964e43671328bf6f9b1d75703103db8

Wicket now logs a stacktrace on error level if you add something to the 
AjaxRequestTarget that has been removed from the page. I don't think this is a 
good idea, because you're not always in control of what happens in an Ajax 
call. Imagine, for example, the following scenario:
{code:java}
public abstract class MyPanel extends Panel {

    public MyPanel(String id) {
        super(id);

        this.setOutputMarkupId(true);

        this.add(new AjaxLink("myLink") {
            public void onClick(final AjaxRequestTarget target) {
                this.executeImportantWork(target);

                //Display something nice on this Panel
                //...

                target.add(MyPanel.this); //In order to show the nice things
            }
        });
    }

    protected abstract void executeImportantWork(AjaxRequestTarget target);
}

{code}
Now, what if the user of MyPanel actually adds it to the Page and then, in 
executeImportantWork, removes it and replaces it with smth else? Then, you get 
an error in the log, everytime someone clicks myLink.

In this case, it could be fixed by adding MyPanel.this to the target *before* 
calling executeImportantWork, in which case you also get something in the log, 
but without stacktrace and on WARN level, namely this: "Component not rendered 
because it was already removed from page", but this is far less invasive than 
the scary stacktrace on ERROR level.

I suggest changing the level of the message to WARN and removing the stacktrace.


> Error logging in AjaxRequestHandler is too strict
> -------------------------------------------------
>
>                 Key: WICKET-6600
>                 URL: https://issues.apache.org/jira/browse/WICKET-6600
>             Project: Wicket
>          Issue Type: Improvement
>          Components: wicket-core
>    Affects Versions: 8.0.0
>            Reporter: Gert-Jan Schouten
>            Priority: Major
>
> As a result of these three commits:
>  * 
> [https://github.com/apache/wicket/commit/dec1293da853357f483eadec4616c9758ea025f9#diff-8964e43671328bf6f9b1d75703103db8]
>  * 
> [https://github.com/apache/wicket/commit/3be55cf5a24982cfca400225aabb6c4296859a7d#diff-8964e43671328bf6f9b1d75703103db8]
>  * 
> [https://github.com/apache/wicket/commit/830fb15c333ad6099f466b2344a84b513690db86#diff-8964e43671328bf6f9b1d75703103db8]
> Wicket now logs a stacktrace on error level if you add something to the 
> AjaxRequestTarget that has been removed from the page. I don't think this is 
> a good idea, because you're not always in control of what happens in an Ajax 
> call. Imagine, for example, the following scenario:
> {code:java}
> public abstract class MyPanel extends Panel {
>     public MyPanel(String id) {
>         super(id);
>         this.setOutputMarkupId(true);
>         this.add(new AjaxLink("myLink") {
>             public void onClick(final AjaxRequestTarget target) {
>                 this.executeImportantWork(target);
>                 //Display something nice on this Panel
>                 //...
>                 target.add(MyPanel.this); //In order to show the nice things
>             }
>         });
>     }
>     protected abstract void executeImportantWork(AjaxRequestTarget target);
> }
> {code}
> Now, what if the user of MyPanel actually adds it to the Page and then, in 
> executeImportantWork, removes it and replaces it with smth else? Then, you 
> get an error in the log, everytime someone clicks myLink.
> In this case, it could be fixed by adding MyPanel.this to the target *before* 
> calling executeImportantWork, in which case you also get something in the 
> log, but without stacktrace and on WARN level, namely this: "Component not 
> rendered because it was already removed from page", but this is far less 
> invasive than the scary stacktrace on ERROR level.
> In other cases, however, it may be more difficult to fix, for example when 
> you're not in control of the source code of MyPanel. Sure, one could argue 
> that you shouldn't alter the page hierarchy in the executeImportantWork 
> method, but usually, there's always a certain edge case where 99% of the 
> times, no page hierarchy change occurs, but that 1% of the times, it is 
> necessary.
> Wicket is a component based framework, so it should be possible to specify 
> components with abstract behaviour and not restrict the user of those 
> components in what they can do in the implementation of that abstract 
> behaviour, especially since, in this case, it does not lead to any actual 
> problems and can safely be ignored with a warning.
> I suggest changing the level of the message to WARN and removing the 
> stacktrace.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Reply via email to