-
Notifications
You must be signed in to change notification settings - Fork 195
Update the module to Platform 3.x #221
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
947106b to
93606dd
Compare
| </property> | ||
| </bean> | ||
|
|
||
| <bean parent="obsServiceTarget" > |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you mind explaining why we removed this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Having this causes an error.
Caused by: org.springframework.beans.factory.NoSuchBeanDefinitionException: No bean named 'obsServiceTarget' available
| <!-- Add here beans related to the web context --> | ||
|
|
||
| <bean id="legacyUiUrlMapping" class="org.springframework.web.servlet.handler.SimpleUrlHandlerMapping"> | ||
| <property name="patternParser"><null/></property> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why add it if the value is null?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Caused by: org.springframework.web.util.pattern.PatternParseException: No more pattern data allowed after {*...} or ** pattern element
This is a hack I used to avoid PatternParseException.
So what's happening is that there are two "org.springframework.web.servlet.mvc.SimpleControllerHandlerAdapter" beans registered in the context. One we explicitly register here and one that is implicitly registered here by the use of the The bean we explicitly create gets named I'm not entirely sure what happened, but this commit is part of Spring 6 and it will just remove the implicit alias of So a couple of things suggest themselves:
Personally I think 3 is the cleanest thing, but may require more tweaks, e.g., to ensure that the new |
@ibacher I tried both 1 and 3. However, I am still seeing the error. Here are the PRs for them. |
|
Well, I stupidly assumed that the XML stuff wouldn't interfere with the |
|
Anyways, @wikumChamith I added a commit here and to openmrs/openmrs-core#5374 that fixes that issue in a way I'm happy with. There are still some test failures but they aren't caused by this issue. Btw, we should take every opportunity available to dump XML Spring configurations if possible. There were some issues with the services wrapped in |
|
PR to fix test errors on ChangePasswordFormControllerTest: openmrs/openmrs-core#5394 |
|
I put a comment on that pull request. |
|
@wikumChamith did you eventually figure this out? |
|
@wikumChamith in that failing test, try changing |
|
@dkayiwa, @ibacher the module is compiling now, but I’m seeing an error when I run the module: To resolve this, I removed the taglib dependencies from core and used org.glassfish.web:jakarta.servlet.jsp.jstl in this module. However, I’m still seeing the same error. We’re currently using the JstlCoreTLV validator, but since it now uses Jakarta EE, removing it didn’t fix the issue either. Is there anywhere else that might still be using a javax-era taglib that I may have missed? |
|
@wikumChamith did you figure this out? |
Nope, still trying to sort it out. Any suggestions?? |
| Concept concept = cs.getConcept(5497); | ||
| //sanity check, the current preferred Name should be different from what will get set in the form | ||
| Assert.assertNotSame("CD3+CD4+ABS CNT", concept.getPreferredName(britishEn).getName()); | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why you did not use a different pull request for these formatting changes?
| ProgramFormController controller = (ProgramFormController) applicationContext.getBean("programForm"); | ||
| controller.handleRequest(request, new MockHttpServletResponse()); | ||
|
|
||
| Context.clearSession(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What failure is the above addressing?
|
@wikumChamith because |
@dkayiwa, that also gives a similar error: I also tried deleting all the |
|
Even after removing the validator? |
Yes |
|
Can you open a pull request for the changes you did in core for this? |
|
|
How are you running it? I seem to be getting a different error: |
I tried running it with both the SDK and |
…and updating dependencies
… for Jakarta EE compatibility.
…d centralize fallback logic.
|
@dkayiwa what is the reason for the force push :) ? |
|
@wikumChamith i just clicked the update branch button 😊 |
This would be the case, but only within the scope of the Not sure what to suggest here. We could try:
|
|
@wikumChamith do you still get the 404 errors when you put an order value less than 100 for this urlMapping https://github.com/openmrs/openmrs-core/blob/master/webapp/src/main/webapp/WEB-INF/openmrs_static_content-servlet.xml#L27 This one legacyUiUrlMapping has 100 |
Yeah, I tried with an order value of 50. |
|
Did the error change from the server side stack trace? |
|
Yeah, it did. It started throwing a I thought this was caused by the leading slash in the jspViewResolver prefix, so I removed it. After that, I started getting 404 errors again, and I am no longer seeing any errors in the server-side logs. |
|
Can i look at the changes that you made? |
@dkayiwa draft PR: openmrs/openmrs-core#5510 |
No description provided.