LPD-19992 Validate id and name attribute in links according to the spec #4070
Conversation
|
CI is automatically triggering the following test suites:
|
liferay-continuous-integration
added
ci:test:sf - pending
ci:test:sf - success
and removed
ci:test:sf - pending
labels
✔️ ci:test:sf - 1 out of 1 jobs passed in 4 minutesClick here for more details.Base Branch:Branch Name: master Sender Branch:Branch Name: LPD-19992 1 Successful Jobs:For more details click here. |
|
ci:test:relevant |
|
Jenkins Build:test-portal-source-format#8378 Jenkins Report:jenkins-report.html Jenkins Suite:sf Pull Request:liferay-frontend#4070 Testray Routine:EE Pull Request Testray Build:[master] ci:test:sf - antonio-ortega > liferay-frontend - PR#4070 - 2024-03-15[08:53:13] Testray Build ID:2550775801 Testray Importer:publish-testray-report#31553 |
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#3974 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#4070 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - antonio-ortega > liferay-frontend - PR#4070 - 2024-03-15[08:57:54] Testray Build ID:2550935208 Testray Importer:publish-testray-report#31555 |
|
ci:test:relevant |
|
LGTM ;) |
❌ ci:test:stable - 32 out of 34 jobs passed❌ ci:test:relevant - 196 out of 255 jobs passed in 4 hours 12 minutesClick here for more details.This pull is eligible for reevaluation. When this upstream build has completed, using the following CI command will compare this pull request result against a more recent upstream result: ci:reevaluate:1347401_5956 Base Branch:Branch Name: master Upstream Comparison:Branch GIT ID: 5523007784929fb2609e594ca2e664dfb8b7535a ci:test:stable - 32 out of 34 jobs PASSED2 Failed Jobs:
32 Successful Jobs:ci:test:relevant - 196 out of 255 jobs PASSED59 Failed Jobs:
196 Successful Jobs:For more details click here.Failures unique to this pull:
Test bundle downloads:
|
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#5956 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#4070 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - antonio-ortega > liferay-frontend - PR#4070 - 2024-03-18[01:24:14] Testray Build ID:2551608507 Testray Importer:publish-testray-report#23418 |
There was a problem hiding this comment.
@antonio-ortega The change in this PR is perfectly fine, but typically we would do review on liferay/liferay-ckeditor#208, before the relevant code is merged. Only after review we do release and version update in DXP. @dsanz Did we change process?
Looking at merged code, I am not to fond of the change in liferay/liferay-ckeditor@2d2e4b5#diff-5492248584039a64baab3a856813c85392795066c7312a565ae463195bb5ef1b:
alert(
Liferay.Util.sub(
Liferay.Language.get('there-was-an-error-when-loading-the-x-field'),
fieldName
)
);My doubts here are:
Liferay.Util.subandLiferay.Language.getare DXP-specific. Changes in patches have always been platform agnostic. It might make sense to change this practice, but did we agree on this?- If we are starting to use DXP-specific utilities, we should avoid using vanilla
alert. We should be usingLiferay.Util.openAlertModal.
A minor thing also, we would typically split changes in code to (1) commit with changes and (2) artifacts update. I was only able to see the changes because of the patch commit.
|
Hey @markocikos , Thanks a lot for your comments. They are very useful to me. First of all, I'd like to apologize for merging my pull request straight away. That was completely my bad. I had sent a previous pull request to However, those two comments you've made here to my pull request make sense in the context of sending that code to I'll send a new pull request with changes we agree here. Now, going to your doubts:
I'd like to ask @dsanz about this, since in a private conversation (and sorry for bring it public) he said something like:
and I directly assume I could make use of it.
It makes sense to me. I was just trying to be consistent with link plugin itself where other warning are shown with vanilla alert.
I'm not understanding this one. If we are talking about the pull request sent to |
Yep, Dani correctly said that, I'm sure this works.
Yeah, that makes sense to me. Could we keep the text in the alert defined in the same way like in plugin, maybe showing some generic error message?
My bad, you're right on this one! There are no commits outside patches. I was thinking of changes on repo that are outside of |
@dsanz What do you think? |
|
Nice discussion guys ❤️
Nop, things are the same
I'd not be much concerned with the appicability of our ckeditor fork outside of liferay. Unless I'm missing any use case, this is more a requirement on "agnosticity" we added to foster separation of concerns. Some time ago I had similar concerns when adding clay-specific classes to the repo, yet we ended up allowing them. In the end we build our customizations to make them work in DXP. That's not to say we must abuse them, e.g. doing bad practises. But in the case of To support this reasoning, we've never changed the translation files in So in this case I do support using this mechanism under the assumption that:
|
|
Thanks for sharing your thoughts, @dsanz . So shall I forward this pull request? We also mentioned about changing error message to show something generic, but I don't know whether that's actually related to the idea of avoiding using
And I think it's better for final users to know which fields are failing rather than just getting "Invalid value". |
|
ci:forward |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
|
Skipping previously passed test suites:
|
|
Jenkins Build:test-portal-acceptance-pullrequest(master)#4070 Jenkins Report:jenkins-report.html Jenkins Suite:relevant Pull Request:liferay-frontend#4070 Testray Routine:EE Pull Request Testray Build:[master] ci:test:relevant - antonio-ortega > liferay-frontend - PR#4070 - 2024-04-01[04:27:00] Testray Build ID:2557241470 Testray Importer:publish-testray-report#31807 |
|
ci:forward:force |
|
CI is automatically triggering the following test suites:
The pull request will automatically be forwarded to the user
|
|
Skipping previously completed test suites:
|
|
All required test suite(s) completed. |
|
Pull request has been successfully forwarded to brianchandotcom#148712 |
Hey,
In the end, we decided to include this change as a patch in CKEditor, so now we just need to bump version in portal.
Thanks.
Regards.