phpunit now passes on both branches.
Tests are passing for 4.x. They are failing for 8.x-3.x, but they are failing on the dev branch too. The merge request here fixes phpcs on that branch.
liam morland → made their first commit to this issue’s fork.
All tests are now passing except cspell and eslint.
liam morland → made their first commit to this issue’s fork.
Thanks.
Can we get a new release, with PHP 8.4 support? Other modules depend on search_api
, so this is holding up PHP 8.4 support in those module too.
It seems likely that $rows
doesn't have to be generated either. That was put out of use in
#3084714: Render with Views API more properly. →
, commit b9e2350.
liam morland → created an issue.
liam morland → made their first commit to this issue’s fork.
liam morland → created an issue.
Rebased; phpcs now passes.
Please add comments explaining what each part of the new function does.
Are the two changes to #markup
necessary?
liam morland → created an issue.
Changes are correct. Tests pass on max PHP.
This should include a list of all allowed values for #type
and what properties they have. There was excellent documentation like this for Drupal 7.
Tests are passing on max PHP.
liam morland → created an issue.
liam morland → created an issue.
liam morland → created an issue.
Rebased. phpunit passes.
I have rebased. Tests do not pass.
Please try the fix in that branch. We may be able to cherry-pick it onto 6.2.x.
Thanks for the patch. Please put it in an issue fork and merge request.
liam morland → created an issue.
liam morland → created an issue.
Please put the patch into a merge request targeting 3.0.x.
Please put the patch into a merge request targeting 3.0.x.
I think you can do this with an entity query. Query for terms that have the given term as a parent.
Please rebase on 3.0.x.
Please rebase on 3.0.x.
This patch contains the current state of the merge request.
This patch should go into an issue fork and merge request.
I would find it more convenient to leave the config as it is until the next time the config is saved. It defaults to the current values so nothing changes.
Re "these tests were extremely broken prior to this merge request": The current merge request adds the .gitlab-ci.yml
file. I suggest committing
📌
Add testing configuration
Active
. This adds .gitlab-ci.yml
and gets the tests passing. Use that issue to get tests passing and then ensure that they keep passing in the current issue.
@#18 Does the patch in the merge request fix it for you?
No reply in 2 months.
Done
Thanks for the patch.
Please put the patch into an issue fork and merge request.
This makes sense to me. Can you add a test?
Please fix the coding standards issues. Can you add a test?
Thanks for the patch.
Thanks for the patch. This is a good idea.
Are you able to add a test that demonstrates this? That would help to get it solved and prevent a regression.
liam morland → created an issue.
Is this actually a problem? The conclusion in #2990879: 'resource' is a soft reserved keyword as of PHP version 7.0 and should not be used to name a class, interface or trait or as part of a namespace → was that this is a false positive from PHP compatibility checker.
Link issues in summary.
I know the code in #3 would not work. I wrote that as an example of what I would like to be able to do in order to avoid executing the View twice.
I think being more efficient is more important than not looking weird.
Why the call to function_exists()
? mb_convert_encoding()
has been in PHP for ages.
Why the call to function_exists()
? mb_convert_encoding()
has been in PHP for ages.
Thanks. I would rather it not be on the page at all.
I'd like to switch this to a feature request: Add a function which returns a printable value if there are results in the View and otherwise returns NULL. That would allow the new function to be used in place of drupal_view_result()
in code like in #3.
So, is my code snippet the recommended way to do it?
To save rending the View twice, I thought that something like this would be possible:
{% set view_content = drupal_view_result('related', 'block_1') %}
{% if view_content is not empty %}
<section>
{{ view_content }}
</section>
{% endif %}
git bisect
helps you find it. If it's broken now but there is some previous version that worked, it will help you to search for the first commit between the two where it is broken. git bisect
will checkout one version after another and you just have to test and tell Git if the current one is good or bad.
Can you tell which commit caused this? You can use git bisect
to figure it out.
liam morland → created an issue.
One use case: Complete a Webform, download a PDF filled with values from the Webform, sign it, and mail it in. This gives a signed paper form with data already in your system.
I have made more fixes and phpcs now passes. One of the changes renames a property, which could cause issues for anyone who extended this class. That change could be left-out for now and an exception added if you want. Please let me know.
Did you add a visibility declaration (like protected
) to all the properties? It should be added only to those properties that are defined in this class. Any inherited properties should not get a visibility declaration.
Interesting. What if you are on the feature branch that tracks the merge request branch in the issue fork and just run git push --force
without any other params? That is what I usually do.
liam morland → created an issue.
liam morland → created an issue.
Testing is now enabled.
You could rebase the branch and force-push it.
MailManagerInterface::mail()
returns an array of information about the message and "the 'result' element will contain the success indicator of the email". For your code to work, it would need to return FALSE when mail fails. It doesn't.
Is it fixed if you use 6.2.x with merge request !470?
I don't recall the problem that I was facing last May, just that I had traced it to the missing newlines.
POSIX says they should be there. Is there any standard that says they should not be there?
If a CSV library does not work properly when the newline is there, that sounds like a bug in the library.
Ending every line with newline is a requirement of POSIX. The definition of a line is "A sequence of zero or more non- characters plus a terminating character." So, unless the file contains zero lines, it will end in a newline.
Thanks for the patch.
It seems to me that the default separator
should stay the same so that existing installs do not change.
Sorry, I can't quickly find examples. It does make sense to me. On the other hand, as long as the versions for which support is being dropped are no longer supported, perhaps it doesn't matter. This merge request puts the minimum at Drupal 10.3 and that is the oldest supported.
In other issues, people have suggested that dropping old version support should not be done in a patch-level release. To follow that, these changes should go in 2.0.x. There could be a 2.1.x which drops supports for CKEditor 4 and perhaps Drupal 10 as well.
liam morland → created an issue.
Please confirm that the problem does not exist in 6.3.x.
I have removed the property. I expect that will fix it. Please test.
This will probably require a ticket in the Drupal.org infrastructure queue.
You can checkout the branch of the merge request, run git reset --hard 6.3.x
, make your changes, then git push --force
. That will re-use the current merge request. It will automatically create a tag pointing at the old commits so they are there to refer to. Nothing gets lost.
/rebase
Thanks for the rebase. By the way, you can just force-push to the same branch as the merge request instead of making a new merge request. It will automatically create a tag pointing at the old commits, so there are there to refer to. Nothing is lost.
Please rebase onto the latest 6.3.x; that will fix some errors. The comments added in src/WebformRequest.php
should not have an empty line between the @param
statements. Unless this method doesn't match what is in WebformRequestInterface
, it can use {@inheritdoc}
.
I note that the tests still don't pass on PHP 8.4. This may have to wait on other modules updating.
Thanks for the patch.
I see a yellow triangle here, lines 244 and 253:
https://git.drupalcode.org/project/webform/-/merge_requests/585/diffs#49...
There cannot be a default value for any param that is followed by a param that does not have a default. Example:
public function getUrl(EntityInterface $webform_entity, ?EntityInterface $source_entity = NULL, $route_name, array $route_options = []) {
$source_entity
has a default, but $route_name
does not. To fix, remove the default from $source_entity
.
This patch as-it-is has the problem that it breaks any code that extends MessageNotifierBase
.
This is a very big patch and I am feeling some pressure to have full release for 6.3.0. Perhaps we should put this onto a 6.4.x branch.