MR !9765 seems to be the closest to a fix of the two merge requests. The other MR breaks several tests. This MR only breaks one PHPUnit Unit test.
However it does not include a failing test, in fact a test is still required. Also the existing code needs fixed so it does not break any existing tests.
Probably this should be reviewed before changing to RTBTC.
Have added more detail to the 'steps to reproduce' section of the issue description.
oily → made their first commit to this issue’s fork.
Tried to fix Javascript lint. Search and replaced $exposed_form with camelcase $exposedForm. Reduced lint errors from 20 to 18.
oily → made their first commit to this issue’s fork.
Remove 'Needs tests' tag.
Remove 'Needs tests' tag.
There is now a failing test that turns green. Changing status to 'Needs review.'
Hiding MR 9584 because MR 10005 is now 'where the action is'.
The test coverage in 10005 is not adequate because manually running the 'test-only' test passes whereas it should fail.
It needs to be discovered whether the test passes because the issue is now fixed or the test is wrong.
oily → changed the visibility of the branch 2937759-if-computed-view to hidden.
oily → made their first commit to this issue’s fork.
Test-only test fails as it should. Pipeline is all green. Change to 'Needs Review'.
Renamed test view in accordance with naming pattern. Removed UUID at top of the test view. Attempt to fix multiple broken tests.
oily → made their first commit to this issue’s fork.
oily → made their first commit to this issue’s fork.
Fixed PHPCBF. Looking at the comment on the MR, the function is used twice, not once, so seems okay. That leaves a comment that needs to be improved. There is also a CSpell lint error from pipeline.
Test is moved to the node module.
@joel_osc can you please clarify this,
This would not work because you would end up questions for Doctors mixed in.
I have added the tag Needs steps to reproduce as people will need to know how to create a quiz via the UI and configure the various things like questions and vocabulary to test out the problem via the UI.
oily → changed the visibility of the branch 3281934-plugin-entityreference-default to active.
oily → changed the visibility of the branch 3281934-validation-reference-access-language to hidden.
oily → changed the visibility of the branch 3281934-validation-reference-access-language to active.
oily → changed the visibility of the branch 3281934-plugin-entityreference-default-11.x to hidden.
oily → changed the visibility of the branch 3281934-plugin-entityreference-default to hidden.
oily → changed the visibility of the branch 3281934-validation-reference-access-language to hidden.
oily → made their first commit to this issue’s fork.
Test can be reviewed. Set to 'Needs review'.
Changed hard-coded uuid's in the kernel test with generated uuid's.
oily → made their first commit to this issue’s fork.
@codebymikey That sounds good but a CR is required all the same then can change to 'Needs review' and get it reviewed. The CR should reflect what is in the MR. Could update the description to state what has been actually done in the MR as distinguished from several possible approaches.
@codebymikey I think your plan at #36 should be a new child issue. It seems a feature request whereas this issue is a bug. The current MR fixes the bug.
Screenshots tag removed.
Resolved PHPSTAN recommendations in the MR. Tests are passing. Looks like a change record can close this.
oily → made their first commit to this issue’s fork.
I have added the CR as requested and added the return types to the MR. Setting back to 'Needs review'.
I am adding a change record.
Perhaps
An Ajax-related error has occurred. Check your browser's console for further details.
RE #19 and #20, @james.williams, I believe a more clinical way of changing this error message is by an event listener, listening for kernel events ie in a custom module. If you are into Symfony the Symfony docs might help. I believe you can listen for the Error being triggered, intercept it and override the message.
RE: #20 and @quietone's proposed message
Something went wrong. Your browser's developer console contains more details. Contact the site administrator if the problem persists.
I don't like the 'Contact the site administrator if the problem persists.' It is too generic. There are lots and losts of other core error messages that you could append that sentence to. And IMO it would not be helpful. It sounds a little opinionated. 'Oops' is a little patronising at the other end of the scale. Opinionated in the sense that although every Drupal site has a user 1 'Site administrator' account, there will be plenty of organisations using Drupal who do not have a clear IT hierarchy with an identifiable 'Site administrator.'
It would be interesting to do a survey but I would guess that in a typical charity or SME business the whereabouts and identity of the 'Site administrator' might be unknown if not unknowable. There will often be several possible 'Web savvy Samantha or Bobs' or maybe the guy at the next desk.
I think the message should be pitched at those who know what browser developer tools are and how to access the console. If they dont know then we can assume they will reach out to others for help without our suggesting it.
I have done further manual UI testing of the error message(s). It is clear that the description is confusing and misleading. I have made further edits.
There is a problem with the existing description. I have made some edits to the description already but this one remains.
'When you try to reference a media object to an entity it sometimes throws the error...'
This sounds to me like the error occurs from time to time randomly. But that is not the case. When the user is trying to add a YouTube video (i have not tested with Vimeo) in the background httpclient validates the url by issuing a GET request to that Url. If the Url does not correspond to a currently publically available YouTube video as proven by the result of the GET request, validation fails and the error message appears.
On the other hand, when a YouTube video has been successfully added as an Oembed resource in the media library and then inserted into a node but the video is then deleted or made private by the YouTube owner, then this error is shown in the node →
@smustgrave The original description was that the url needs to be included in the error message. After reproducing the error I discovered that the description is not clear enough. What in fact happens is that according to form validation functionality the Oembed resource field is highlighted in red. See Oembed url highlighted in red → in combination with the message being printed in the standard message position of the page.
This led me to think that either the existing form validation mechanism which is similar to many other form libraries out there on the web is not adequate and so form validation itself needs to be fixed, or it does its job in this case.
From there I concluded that the problem is not that the url is not printed in the error message which would seem like unnecessary duplication, since the url is contained in the field that has been highlighted in red, rather the problem is that the error message itself is not clear. In as few words as possible I have tried to rectify the message. The existing message does not say what action needs to be taken. The new message makes it clear IMO that it is the url that is at fault. Therefore the url should be checked as it appears not to exist.
I see no test in the MR. A test is required.
@eugene bocharov I had a look at possible places in core to create the test, looked inside the translation system. I could find no obvious place to put a test whether a unit or other type of test.
I've added test just for returning value type
That seems the right approach.
What @quietone says in #18:
So, I do think we should take the time to add a test so this does not happen in the future.
That still sounds correct. Test changing the language eg from default 'en' to 'fr' and asserting that the day of the week in that language is returned. eg 'lundi'.
The concerns about regressions seem reasonable. But the rule is to create a test for the functionality being implemented whethere it is a new features, a reversion to old code. In terms of creating a failing test, those matters are immaterial. The test seems to me to be missing and should be added. It should fail before the MR is applied and pass after it is applied.
If such a test is in place already, then I have misunderstood.
The error message has been replaced by: 'Check the url. It does not correspond to any remote oEmbed resource.' The existing test has been updated to assert the new error message. All tests are green. Changing to 'Needs review'.
Testing this issue in a local Drupal 11.x site. see screenshot → showing how a YouTube video saved in a node when the video was publically available but has since been deleted on YouTube is handled.
A message similar to this, also set in an autogenerated error thumbnail image is displayed if the setting for a YouTube video is changed from 'public' to 'private'. In neither case is a message displayed on the page.
A message IS displayed if the user, for example, tries to edit the url to a non-existent url and then save the node.
oily → made their first commit to this issue’s fork.
Tests are all green.
Please review the new change record → .
Good spot. Have restored the attribute.
@james.williams Looking at patch #39 here there is some useful looking test code usable in the test for #3449408: Use interface language on the add URL alias form
@james.williams Thank you, the new 'steps to reproduce' make things nice and clear. It appears that the test should be quite simple and specific: the default setting of one particular field in the form.
Another way of testing could be to check whether an alias added through the standalone URL alias form is given the same language as an alias created from a node creation form. At the moment; they would be different. That is the angle that #2484411: Manual path aliases are not the same as aliases on the node form on single-language sites looks at this problem from. I believe both of these issues can be solved with the solution that I added in MR !8175 for this issue.
This all sounds good to me.
Fixed csSpell failure reported by the bot
oily → made their first commit to this issue’s fork.
@james.williams
I also believe the monolingual scenario should be covered:
But that is what issue #2484411 is about is it not? So should be fixed in that issue?
At #8 @smustgrave requested a 'simple test'. Is that not now in place?
At #9 you linked #2484411. It will help if the issue description is filled out especially the 'steps to reproduce'. But if the test coverage covers this issue then perhaps #2484411 can be dealt with as a child issue. Under it the test coverage can be extended.
Applied bot fix.
After throwing InvalidArgumentException for the DatabaseStorage::exists and DatabaseStorage::read methods, I found that the former breaks an existing kernel assertion/ test. So I have reverted to the existing DatabaseExceptionWrapper class to fix the test. The tests are back to green. Setting back to 'Needs review'.
@smustgrave I see several things in the threads. I note this issue → . However if you search through core for usages of InvalidArgumentException you will find that by far the majority are not extending from the base class. Issue 2654936 concerns the Rules module. In that issue it is stated that not extending from base classes like InvalidArgumentException is 'bad OOP'. However if true then they could have created a child issue: to remove all the unextended uses of InvalidArgumentException in core. I am unaware of any initiatives to do that.
Have added test coverage. Changing to 'Needs review'.