I have been searching through the views module to work out where to put the validation for the views page display form. I think it would be in DisplayPluginBase.php perhaps before line 2533. That is where the 'more link' path? is validated. So could add validation for the page display path above or below it? The more link validation seems to have useful error code can be utilised.
Added code review with recommendations. Not sure why PHPCS is not enforcing it but 'Implements hook_' docblocks seem to be always placed just above the hook attribute of the function (i checked by grepping 'Implements hook_').
Re: #11 I have added a @todo. To complete the todo may require the validation to be in place to test on the exact validation error text.
I have added test coverage. Leaving the test coverage loose (assertNotContains(200)) as I do not think the validation is in place yet. Once it is, might be best to tighten the test to test for the validation string?
RE: #90 The new hook seems to be adding entity fields ie nothing to do with the account form fields..
I see this in the UserHooks.php file in the user core module.
/**
* Implements hook_entity_extra_field_info().
*/
#[Hook('entity_extra_field_info')]
public function entityExtraFieldInfo(): array {
$fields['user']['user']['form']['account'] = [
'label' => $this->t('User name and password'),
'description' => $this->t('User module account form elements.'),
'weight' => -10,
];
$fields['user']['user']['form']['language'] = [
'label' => $this->t('Language settings'),
'description' => $this->t('User module form element.'),
'weight' => 0,
];
if (\Drupal::config('system.date')->get('timezone.user.configurable')) {
$fields['user']['user']['form']['timezone'] = [
'label' => $this->t('Timezone'),
'description' => $this->t('System module form element.'),
'weight' => 6,
];
}
$fields['user']['user']['display']['member_for'] = [
'label' => $this->t('Member for'),
'description' => $this->t("User module 'member for' view element."),
'weight' => 5,
];
return $fields;
}
This new hook could have a bearing on this issue.
RE: #88
I edited #87 quite a lot before I saw #88 mainly in agreement with #88. Starting to get my head around it. I tracked down your comment in the related issue. So these optional form elements will now be always present in the render array but as 'hidden' fields when not needed? It sounds an enhancement if it is likely to be a restricted list of specific fields for the foreseeable future. If it were likely to grow to more than a few fields might not be an enhancement..
RE #86 Yes, after running the test-only test seems pointless since no test coverage so far.
Rebased 221 commits.
Test-only test passes:
ℹ️ Changes from faec1421c65a694b320f6c807c9546b4dde9482d
core/modules/user/src/AccountForm.php
If this list contains more files than what you changed, then you need to rebase your branch.
1️⃣ Reverting non test changes
grep: warning: * at start of expression
grep: warning: * at start of expression
↩️ Reverting core/modules/user/src/AccountForm.php
grep: warning: * at start of expression
2️⃣ Running test changes for this branch
Exiting with EXIT_CODE=0
Running after_script 00:01
Running after script...
$ sed -i "s#$CI_PROJECT_DIR/##" ./sites/default/files/simpletest/phpunit-*.xml || true
sed: can't read ./sites/default/files/simpletest/phpunit-*.xml: No such file or directory
Uploading artifacts for successful job 00:00
Uploading artifacts...
WARNING: ./sites/default/files/simpletest/phpunit-*.xml: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-2058319)
WARNING: ./sites/simpletest/browser_output: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-2058319)
ERROR: No files to upload
Uploading artifacts...
WARNING: ./sites/default/files/simpletest/phpunit-*.xml: no matching files. Ensure that the artifact path is relative to the working directory (/builds/issue/drupal-2058319)
ERROR: No files to upload
Cleaning up project directory and file based variables 00:01
Job succeeded
I thought it was possible to use cspell in a 'context-aware' fashion. So if word is only used in css only, can the cspell syntax handle that?
Resolved merge conflict.
Updated IS.
#19 A rebase is required. Because I seem to upset people by doing rebases I didnt do it. No one triggered the test-only test. I read through the code changes. The test looks as tests go very simple and uses the same method as the code change. There seems little room for the test coverage to be inadequate, therefore. No one triggered the (manual) test-only test. I wanted to make sure the test failed so did more than just look correct. I also edited the IS.
Pipeline is green. Test-only test fails as it should.
The new CR draft is good. RTBTC.
@nixon RE:10 The source of my confusion was the idea of 'Include files'. If it had said '.inc' files that makes immediate sense to me.
RE: #10 I also edited the CR.
RE: #6 updated IS.
Made the CR a bit clearer, though now Im not sure if @berdir means '... loadAllInclude() can be used in a loop for modules'? maybe 'used in a loop for includes'?
RE: #4 @nicxvan I cannot see where that text appears? Has it been corrected now?
Updated IS according to #21 and #24.
RE: #23 @smustgrave It could help if someone greps the codebase? They'd return i think a lot more with 'check' than 'validate'. Also the IS distinguishes between the 'checking' done client-side by the browser to the Validation component validation done 'server-side'. We want the field value to be validated as opposed to 'checked'? Also 'checking' can create muddle as we check checkboxes in forms api.
RE: #16: @benjifisher Thank you for getting to the heart of this.
RE: #14 @benjifisher Yes I could have tried that. But not sure how that would have worked when there is no doc standard? enforcing the line spacing change I made. But there seems a de facto 'standard'. I suppose i self-RTBTC'd which does happen. Interesting to hear your points. But deleting a few blank lines which has no PHPCS/STAN implications, but does seem a slight enhancement seems okay to self-RTBTC..
Adjusted the empty lines to group together the related tabs/ routes (in this case there are 2x groups): this 'technique' is used in other core *.links.task.yml files in core modules including system and block.
Are there not other places where this fix can be applied? E.g. announcements.js and states.js in the misc folder? There appear to be several instances in those files where the same sub-optimal syntax is used. Should they be included in this issue or in a follow-up?
Are there not other places where this fix can be applied? E.g. announcements.js and states.js in the misc folder?
Line 227 of UrlHelper.php, removed comment that could confuse the meaning of the comment addition in this MR. Cannot see any purpose in it at all except to confuse people.
Corrected comment. We are validating the date, not 'checking' it. Similar idea but let's be consistent and specific!
Fix merge conflict.
Fixed merge conflict.
@acbramley Okay. I should have said 'knock-on' effects described in comments #1 - #7 involving 'contrib' modules and various hooks. Perhaps those comments are not relevant now? They date from 2019. Or are they addressed in the MR?
Although the IS mentions sort of 'knock-on' effects of the main issue, might help to distinguish which of those 'knock-on's' getting 'knocked on the head' by this issue. Are there any follow-ups required?
Attempt to simplify the title after reading through the comments.
I just hid a branch because the ...11.x branch is the active one..
oily → changed the visibility of the branch 3326684-deprecated-function-mbstrtolower to hidden.
Added 101 commits from source branch. Re-ran failed functional test. Pipeline is now green.
Fix PHPCS. Try to fix broken kernel test.
I am trying to make the mock callback at the bottom of the test file mock a 'real' callable value ie a string that is the name of a named function. Created a privated function 'namedcallback' and trying to use $this->namedcallback() inside quotes as the callable.
Not sure if I am going about it the right way. Maybe using a random string like 'default value' will work but it would not be a callable string, i think?.. In which case the test is not going to test whether 'default value' is a callable..
Changed the error message: 'string' to 'callable' and the error condition from !is_string() to !is_callable().
Re: #53 Great!
Is this ready to move to RTBTC?
@xjm, Yes, some useful reading in the link. I wondered if the 'pattern' was some ad lib idea : ). It being a standard, that seems fine.
@xjm, Yes, some useful reading in the link. I wondered if the 'pattern' was some ad lib idea : ). It being a standard, that seems fine.
@xjm re: #15
Thoughts?
I grepped for other typehints using the pattern /* array */ like /* string */. Didn't find any. Is it possible that this is creating unnecessary work? Now just follow line of least resistance and move on to other issues..
@berdir I am not sure why line 212 of Renderer.php has /* array */ instead of typehinting the $elements parameter 'array'?
public function render(/* array */&$elements, $is_root_call = FALSE) {
#29 +1. I realised I had come across this before with Symfony textarea field. I wanted to allow the user to enter a list, actually Black Cab runs which students of the Knowledge study and memorise, for example,
Leave on Left Green Lanes
Right Highbury New Park
etc
etc
etc
Forward into Gibson Square
All the runs follow a similar pattern, but they vary a lot in length (how many lines in total).
You have a choice to either present one textbox per line or a textarea for all the lines. Thinking it through I realised that the newlines are easily handled by explode() on the TEXT (string) value. Then custom validation can be applied to the array. The flexibility of the textarea field type, including the length of the field value, is what seems to make it so useful.
I think there might be edge cases where a maxlength value is desirable. But IMO it is best to leave the core textarea field as it is. Developers can create a custom field or use custom validation. But such cases will be relatively infrequent.
@godotislate I have looked at your new kernel test coverage. Can you please advise me if you think I should refactor my test coverage 'borrowing' from your code to take account of new lines? I am not sure my test coverage is currently fit for purpose?
#39 +1. Are we ready for RTBTC? We have test coverage in place, CR seems done.. Pipeline green?
I have had a look at the CR and the MR and recent comments. I note there are 2x todos, 1 in User.php and 1 in Workspace.php. It would seem a follow-up issue could deal with those? Since the 2x timestamps involved in the todos are unusual and need to be handled differently since we are allowing safe handling of NULL values for timestamp in this issue but these 2x todos are when NULL is not currently supposed to be allowed.
Worth taking a look at the phpdocs for the 2x timestamps to see why they should not return NULL and if should throw an exception or how null values are to be dealt with.
Some work needed on the test case. Add newlines to the random text and test how that affects the length?
Fix merge conflict.
Test-only test now fails as it should. Other tests in the pipeline are breaking. Not sure if related.
Fixed PHPCS. Rebased. Now several tests seem broken in pipeline.
Applied several of @longwave's suggestions, PHPSTAN/CS
@smustgrave. I agree. I created a code review as others here did. My review was to change 'string' to 'callable'. I believe my review was ignored while everyone else's was resolved. No idea why.
#49 @smustgrave Because the typehint should not be 'string' it should be 'callable'. If you disagree, please explain why.
Self-RTBTC
Pipleline green.
Change to Needs review.
@acbramley I used the online single file editor. Weird.
Not sure why pipeline failed. Failures dont seem related. It has failed on a one word commit.
Re: #64 Can you please explain why. Please reference the comments I have made. A reasoned response is much appreciated.
@xjm Thank you for #42 but I respectfully disagree: I have been constantly changing my 'point'. It has evolved.
The status is 'Needs review'. Distilled from the above is this:
At line 493:
string|NULL should be callable|NULL
If others disagree then I am keen to hear their viewpoint.
If the callback at line 493 is 'invoked' then it should be typehinted as \Closure, not string|NULL.
Thank you for #36 @xjm. Please refer to #30:
A PHP callable is a PHP variable that can be used by the call_user_func() function and returns true when passed to the is_callable() function. It can be a \Closure instance, an object implementing an __invoke() method (which is what closures are in fact), a string representing a function or an array representing an object method or a class method.,
That seems to be a clear description of PHP callables. It explains the variance in Drupal docs you have highlighted in #36.
The action to take when documenting a callable is straightforward:
- Is it a \Closure? If so it must be an object implementing an __invoke() method. So it is 'invoked'..
- If not a \Closure and does not invoke an __invoke() method, is may be a string representing a function?
- If neither of the above, is must be an array?
Line 508
throw new \InvalidArgumentException('Default value callback must be a <strong>string</strong>,...
makes it clear that the $callback parameter of BaseFieldDefinition::setDefaultValueCallback() is a string callback (see item 2).
Therefore, it is 'called' not 'invoked'. Callables elsewhere e.g. in Drupal API docs (see item 1) are correctly described as 'invoked'.
Re #32 "Plus addressing @joachim's feedback". So why not addressing my 'feedback', too?
#31 @xjm
@oily, Thanks for your interest in this topic.
Everyone else has 'contributed' to this discussion.
... the discussion is out of scope for this issue
How is it 'out of scope'?!
@xjm You are correct that in Drupal API docs eg:
https://api.drupal.org/api/drupal/core%21lib%21Drupal%21Core%21Extension...
there are:
callable $callback: A callable that invokes a hook implementation.
But is that the purpose of the callback in this issue?
If not then best to contradistinguish the chalk from the cheese?
@xjm If you are unconvinced by my arguments (code review), do you want to do a follow-up to change the few instances of 'calling' callables' in the existing Drupal codebase to 'invoking'?
Added code review: callables: called or invoked.